Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: Document interaction of Skip and Error #16502

Closed
bcmills opened this issue Jul 26, 2016 · 8 comments

Comments

Projects
None yet
7 participants
@bcmills
Copy link
Member

commented Jul 26, 2016

go version go1.7rc3 linux/amd64
package todo_test

import "testing"

func TestIssueFixed(t *testing.T) {
    defer func() {
        if t.Failed() {
            t.Skip("Test failed as expected; see issue XXXXXXXX.")
        }
        t.Fatal("Test did not fail as expected; please update issue XXXXXXXX.")
    }()

    t.Log("<no symptoms of issue XXXXXXXX>")
}

func TestIssueOpen(t *testing.T) {
    defer func() {
        if t.Failed() {
            t.Skip("Test failed as expected; see issue XXXXXXXX.")
        }
        t.Fatal("Test did not fail as expected; please update issue XXXXXXXX.")
    }()

    t.Error("<symptom of issue XXXXXXXX>")
}

Expected:

$ go test -v todo
=== RUN   TestIssueFixed
--- FAIL: TestIssueFixed (0.00s)
        todo_test.go:13: <no symptoms of issue XXXXXXXX>
        todo_test.go:10: Test did not fail as expected; please update issue XXXXXXXX.
=== RUN   TestIssueOpen
--- SKIP: TestIssueOpen (0.00s)
        todo_test.go:24: <symptom of issue XXXXXXXX>
        todo_test.go:19: Test failed as expected; see issue XXXXXXXX.
FAIL
exit status 1
FAIL    todo    0.018s

Actual:

$ go test -v todo
=== RUN   TestIssueFixed
--- FAIL: TestIssueFixed (0.00s)
        todo_test.go:13: <no symptoms of issue XXXXXXXX>
        todo_test.go:10: Test did not fail as expected; please update issue XXXXXXXX.
=== RUN   TestIssueOpen
--- FAIL: TestIssueOpen (0.00s)
        todo_test.go:24: <symptom of issue XXXXXXXX>
        todo_test.go:19: Test failed as expected; see issue XXXXXXXX.
FAIL
exit status 1
FAIL    todo    0.031s

I'm guessing this is not actually a bug, but I don't see anything in https://golang.org/pkg/testing/ describing how Skip and Error are supposed to interact. It doesn't seem unreasonable to think that a test could be Skipped after it has already Failed, but that turns out not to be the case as implemented.

Could someone with more in-depth knowledge of the testing package please confirm whether this behavior is intended (and, if so, update the documentation)?

@broady

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

This feels like an abuse of Skip.

For tests like this, I'd rather see a higher level construct. Maybe a function (*testing.T).ShouldFail(string).

@quentinmit quentinmit added this to the Go1.8 milestone Aug 1, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

/cc @robpike also for testing+wording thoughts.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

@broady, I don't think the answer to this bug is more API.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

I agree with @bradfitz. No more API is called for.

Since you're asking my opinion, here it is: A failed test should not be skippable after the failure. The test has run and failed. As @broady says, this feels like an abuse.

Documentation is called for, maybe. It takes a twisted mind to think that you can skip after a test has run.

@mpvl

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

As it is assigned to me, I agree it feels like an abuse.

Furthermore, it would be non-trivial, though not hard, to implement unskipping tests, but people may then get further expectations of parent tests calling Skip undoing failures of child tests and what have you.

@quentinmit quentinmit added the NeedsFix label Oct 10, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

It feels weird, certainly, but it is a very useful construct to be able to say "this test should fail". I don't see another way to do it given the current testing API?

@robpike

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

I don't think it's useful enough to introduce such tricky semantics and vote firmly against this being made official.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 18, 2016

CL https://golang.org/cl/31324 mentions this issue.

@gopherbot gopherbot closed this in a431bdc Oct 18, 2016

@golang golang locked and limited conversation to collaborators Oct 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.