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

Jump to bottom

testing: Document interaction of Skip and Error #16502

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

testing: Document interaction of Skip and Error #16502

bcmills opened this issue Jul 26, 2016 · 8 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@bcmills
Copy link
Contributor

bcmills 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
Copy link
Member

broady 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
Copy link
Contributor

bradfitz commented Sep 9, 2016

/cc @robpike also for testing+wording thoughts.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

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

@robpike
Copy link
Contributor

robpike commented Sep 9, 2016 •
edited

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
Copy link
Contributor

mpvl 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 The path to resolution is known, but the work has not been done. label Oct 10, 2016
@quentinmit
Copy link
Contributor

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
Copy link
Contributor

robpike 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
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 18, 2017
@rsc rsc unassigned mpvl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
7 participants
@bradfitz @broady @quentinmit @robpike @bcmills @mpvl @gopherbot