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: change -run regexp to make / have usual precedence #39904

Closed
dnephin opened this issue Jun 28, 2020 · 18 comments
Closed

testing: change -run regexp to make / have usual precedence #39904

dnephin opened this issue Jun 28, 2020 · 18 comments

Comments

@dnephin
Copy link
Contributor

dnephin commented Jun 28, 2020

The go test -run flag can be used to select multiple root test cases. This example will run the 3 tests which match the regex exactly:

go test -run '^(TestOne|TestTwo|TestThree)$'

As far as I can tell, the logic for handling subtests makes it impossible to do the same with subtest cases. It seems to be possible to run multiple root test cases, with a single subtest case:

go test -run '^TestOne$|^TestTwo$|^TestThree$/^SubTestA$'

But adding anything after the / is assumed to be a sub-sub test, and does not match the same way.

Some other things that I tried, which do not work:

Matches any SubTestA, not just the one from TestThree

^(TestOne|TestTwo|TestThree)$/^SubTestA$

Does not match any subtests

Because of how parenthesis are handled in splitRegex

(^TestOne$|^TestTwo$|^TestThree$/^SubTestA$)

Would it be reasonable to support this with the -run flag? Looking over the matching logic in src/testing it seems like this may not be a small change.

@gopherbot gopherbot added this to the Proposal milestone Jun 28, 2020
dnephin added a commit to dnephin/gotestsum that referenced this issue Jun 28, 2020
If a root test case has failed subtests, only rerun those that have
failed. A new hidden flag is added to restore the old behaviour of
re-running the entire root test case if any subtest fails.

Because of limitations of the 'go test -run' flag (golang/go#39904)
this required running each test in a separate 'go test' process.
@ianlancetaylor
Copy link
Contributor

CC @mpvl

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 28, 2020
dnephin added a commit to dnephin/gotestsum that referenced this issue Jun 29, 2020
If a root test case has failed subtests, only rerun those that have
failed. A new hidden flag is added to restore the old behaviour of
re-running the entire root test case if any subtest fails.

Because of limitations of the 'go test -run' flag (github.com/golang/go/issues/39904)
this required running each test in a separate 'go test' process.
@dnephin
Copy link
Contributor Author

dnephin commented Jul 4, 2020

I see there was some discussion about this in https://go-review.googlesource.com/c/go/+/19122/. I think the | character is not accounted for (none of the tests include a case with one). I can't see any way to make this work splitting the regex by /.

Maybe the same goal can be accomplished by adding [^/]* before and after any non-anchored / in the regex? I guess that only solves part of it. I'm not sure how to have parent tests match the regex so that all subtests are evaluated.

@rsc
Copy link
Contributor

rsc commented Jul 8, 2020

The testing package must have some way to figure out whether the top-level test matches (and should be started), separate from the rest of the expression, so that's why it splits on unparenthesized slashes. But this changes the ordinary precedence of concatenation-with-slash to be higher than alternation (|), which was probably a mistake. Maybe the fix is just to split on |s first.

That is, as shown above

'^TestOne$|^TestTwo$|^TestThree$/^SubTestA$'

is implicitly

^(TestOne|TestTwo|TestThree)$/^SubTestA$

and there's no way to override those implicit parens. Splitting on pipes first would essentially remove those implicit parens.

I'd be willing to try that (splitting first on pipes) at the start of a cycle and see if it breaks anything. Thoughts?

/cc @mpvl

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 8, 2020
@rogpeppe
Copy link
Contributor

rogpeppe commented Jul 9, 2020

Wouldn't that break usage like this, where the intention is to run several of the TestFoo subtests?

go test -run 'TestFoo/First|Second'

I'm not sure that this behaviour can be changed now for that reason.

... and if we decide that the behaviour is ok to be changed, for the record, I still think that things would be better if all these patterns were implicitly anchored (I often get bitten by several tests running when I only intended one because the test name
I've specified is a prefix of others and I've forgotten to add a $ qualifier).

As another possibility for fixing this issue, how about allowing multiple -run flags?

For example:

go test -run '^TestOne$' -run '^TestTwo$' -run '^TestThree$/^SubTestA$'

Another ability I'd like to see is the ability to exclude tests rather than include them, when there's a problematic test, but that's another proposal :)

@dnephin
Copy link
Contributor Author

dnephin commented Jul 12, 2020

A new flag (-run-regex ?) that accepted a regex and did not modify the regex would be one easy way to solve it. I understand there is a desire to keep the number of flags to a minimum, so I imagine this option is not desirable.

Allowing the -run flag to be repeated should work in a backwards compatible way.

I think splitting on | could work, but it would require parens to be added to some values. Assuming parenthesis are handled the same way they are now, which is that any / or | within an open left paren does not cause a split.

TestFoo/First|Second would need to be written as TestFoo/(First|Second).

@rsc
Copy link
Contributor

rsc commented Jul 15, 2020

@rogpeppe, you're right that we'd be changing the meaning of that syntax, but X/Y|Z not meaning "X/Y or Z" is arguably the same kind of bug. You can always write X/(Y|Z) to get that meaning, and most people probably would already (because that's what you'd have to say to get that meaning in a plain text regexp match).

The problem that this change would solve is that A/B|C/D today means A/(B|C)/D, in contradiction to the plain text regexp match, and worse there is no correction, no way to get "A/B or C/D".

The suggestion solution - splitting on pipes before splitting on slashes - more closely respects the text matches, by making the pipe operator a looser binding than the implicit concatenation around slash. And when it's not what you want, parens can always be added. Pipe is the only operator that binds looser than concatenation, so it's the only instance of this problem that we'd need to address.

An alternative would be to go back to exactly the plain text semantics, with the added rule that matching right up to a slash is allowed. That would require extra support in the regexp engine and would still be special, but even more differently special (for example, -run='^X$/^Y$' would not work anymore), so I'm not inclined to go down that road instead.

The current syntax was a bit of a compromise, but it does work well in general, and so the general design might as well stay. But it still seems like we should fix this precedence problem that has no workaround. Yes, it does change the meaning of certain corner cases - by design - but it changes them in a way that can always be overridden with parens (in contrast to today).

@rsc rsc changed the title proposal: testing: support running subtests of different root tests using 'go test -run' proposal: testing: change -run regexp to treat / like concatenation (not higher prec than |) Jul 22, 2020
@rsc rsc changed the title proposal: testing: change -run regexp to treat / like concatenation (not higher prec than |) proposal: testing: change -run regexp to make / have usual precedence Jul 22, 2020
@rsc
Copy link
Contributor

rsc commented Jul 22, 2020

Does anyone object to making the change described in my comment above?

@rsc
Copy link
Contributor

rsc commented Aug 5, 2020

Based on the discussion above, this seems like a likely accept
(for early in cycle, understanding that we might need to back it out).

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Aug 5, 2020
@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Aug 14, 2020
@rsc rsc changed the title proposal: testing: change -run regexp to make / have usual precedence testing: change -run regexp to make / have usual precedence Aug 14, 2020
@rsc rsc modified the milestones: Proposal, Backlog Aug 14, 2020
@firelizzard18
Copy link
Contributor

I am working on vscode-go, and AFAIK the current syntax makes it impossible to run an arbitrary set of tests and sub tests. If I understand the proposal correctly, if this is merged, -run ^(A|B/C)$ would run the base test A and the sub test B/C. That should fix the problem.

@gopherbot
Copy link

Change https://golang.org/cl/343883 mentions this issue: testing: increase alternation precedence

@aclements
Copy link
Member

Maybe I'm misunderstanding, but isn't the proposal in #39904 (comment) in direct contradiction to the regexp/syntax.Cut behavior proposed in #44254?

@firelizzard18
Copy link
Contributor

@aclements I thought so too, but I think it would work if you first Cut by | and then Cut by /

@bcmills
Copy link
Contributor

bcmills commented Jan 20, 2022

FWIW, this change cause some of my existing test scripts to silently omit some tests that previously ran.

I have updated the scripts, but I am somewhat concerned that the failure mode of this change is “silent reduction in test coverage”.

@ianlancetaylor
Copy link
Contributor

@bcmills Do you think we should raise this higher in the release notes? Currently it's only mentioned under "minor changes to the library."

@bcmills
Copy link
Contributor

bcmills commented Jan 20, 2022

I don't think that would really help — once I thought to check the release notes at all, it was pretty easy to find the paragraph about the behavior change.

@rogpeppe
Copy link
Contributor

@bcmills One thing that I've considered proposing is a way to specify an exact list of tests to run, with no regular expressions involved. That would make it straightfoward to (for example) parallelise a single package's tests across instances. It seems that that's what the original poster was after. We could also add the ability to exclude certain tests at the same time. Then we could perhaps revert this change in favour of that. WDYT?

@bcmills
Copy link
Contributor

bcmills commented Jan 21, 2022

@rogpeppe, I think that's mostly orthogonal to this change.

It seems to me that the question for this particular change is whether the benefit making the regexp semantics better match users' intuition is worth the cost of breaking existing scripts that — accidentally or intentionally — relied on the previous semantics.

What I think would help might be to parse and apply the regexp both ways, and emit an explicit warning (or fail the test!) if the regexp would have run additional tests at a prior Go version. That would at least make the change in behavior easier to identify.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants