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

Ensure the test callback sees the sub-test instance #7

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Nov 25, 2019

I found this API usage error while attempting to run t.Fatal or t.Skip
inside a test directive: it's not possible to change the status of an
outer test instance while datadriven.RunTest has instantiated a
sub-test.

Symptom:

 test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test

This patch fixes it by giving the sub-test instance to the
callback. All usages must be updated.

Needed for cockroachdb/cockroach#42250.


This change is Reviewable

@knz knz requested review from tbg and RaduBerinde November 25, 2019 16:11
@knz knz force-pushed the 20191125-test branch 3 times, most recently from 8c8a432 to 061091c Compare November 25, 2019 16:55
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would greatly prefer the breaking API change here (i.e. passing t through) as the current solution leaves the footgun in place (nobody will remember to use d.T in their handler).

@knz
Copy link
Contributor Author

knz commented Nov 25, 2019

ok I'll make an attempt at shaving this yak before I fix cockroachdb/cockroach#42250

@knz
Copy link
Contributor Author

knz commented Nov 25, 2019

I would greatly prefer the breaking API change here (i.e. passing t through)

Done cockroachdb/cockroach#42736

@knz knz changed the title Make the sub-instance of testing.T visible to tests Ensure the test callback sees the sub-test instance Nov 25, 2019
I found this API usage error while attempting to run t.Fatal or t.Skip
inside a test directive: it's not possible to change the status of an
outer test instance while `datadriven.RunTest` has instantiated a
sub-test.

Symptom:
```
 test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
```

This patch fixes it by giving the sub-test instance to the
callback. All usages must be updated.
@knz knz merged commit 1cff705 into cockroachdb:master Nov 25, 2019
@knz knz deleted the 20191125-test branch November 25, 2019 17:21
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Nov 26, 2019
42736: *: bump the `datadriven` dependency and update the calls r=knz a=knz

For use with cockroachdb/datadriven#7.

The `datadriven.RunTest` function uses sub-tests for each directive in
the input file. Since it's not valid to use `t.Fatal`, `t.Skip` etc on
a parent test while there is a sub-test `testing.T` active, the
`RunTest` interface has been updated so that the callback function
gets the sub-test as argument.

This patch bumps the dependency and updates the calls to `RunTest`
accordingly.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants