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

feat: multiple skips #148

Closed
wants to merge 3 commits into from
Closed

feat: multiple skips #148

wants to merge 3 commits into from

Conversation

laurentsenta
Copy link
Contributor

Closes #141

xml: output.xml
html: output.html
markdown: output.md
skips: '["TestTrustlessCarPathing", "TestNativeDag/GET_plain_JSON_codec_from_.*"]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highlight: this is how we skip tests. Note the regexp similar to golang's matching

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same as the following, right?

Suggested change
skips: '["TestTrustlessCarPathing", "TestNativeDag/GET_plain_JSON_codec_from_.*"]'
args: '-skip "(TestTrustlessCarPathing|TestNativeDag/GET_plain_JSON_codec_from_.*)"'

Copy link
Member

@lidel lidel Aug 22, 2023

Choose a reason for hiding this comment

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

Regex support is pretty recent, i think? https://tip.golang.org/doc/go1.20 states:

The go test command now accepts -skip to skip tests, subtests, or examples matching .

go help testflag conforms we now have parity for -run and -skip which is really nice!:

-run regexp
	    Run only those tests, examples, and fuzz tests matching the regular
	    expression. For tests, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any. Note that possible parents of matches are
	    run too, so that -run=X/Y matches and runs and reports the result
	    of all tests matching X, even those without sub-tests matching Y,
	    because it must run them to look for those sub-tests.
	    See also -skip.
[..]
	-skip regexp
	    Run only those tests, examples, fuzz tests, and benchmarks that
	    do not match the regular expression. Like for -run and -bench,
	    for tests and benchmarks, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any.

Both skip and run are useful. run allows more fine-grained control than our predefined presets, which saves time when someone develops a fix and has to re-run specific subset of tests without running the entire suite (not a problem today, but the suite will grow).

@laurentsenta do we need to have dedicated input in github action?
If we could keep this under args we would not have to do anything special to support -run here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also wondering if we even need dedicated run/skip gateway conformance cli flags if we can accomplish the same using the go flags.

If we do, e.g. as nicer to use sugar, I think it might be worth considering converting the custom flag values into regex and passing it to go to avoid having to reimplement run/skip logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks for the reviews and comments,
That work might be a dead end, Sorry for the noise 🤦

I ran more experiments, and not everything makes sense to me, but regular skips should be enough.

@galargh your suggestion would need a different parenthesizing:

(TestTrustlessCarPathing|TestNativeDag/GET_plain_JSON_codec_from_.*) # does not work
(TestTrustlessCarPathing)|(TestNativeDag/GET_plain_JSON_codec_from_.*) # would work

@lidel your issue should be solved with an | and parenthesis:

go test ... -skip '(TestGatewayCache/GET_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore)|(TestGatewayCache/HEAD_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore)'

Probably also:

go test ... -skip 'TestGatewayCache/(GET|HEAD)_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore'

We can drop that PR.

I think the easiest way to think about golang -skip is:

  • create groups with (g1)|(g2) at the top level,
  • then each group is split by / and each part is matched with the go test name.
    • willSkip("regexp1/regexp2", "testX/testY") = match(regexp1, testX) && match(regexp2, testY)

I'm still confused by some aspects of the matching... if you want to dig deeper: https://gist.github.com/laurentsenta/5ed9fe9954605eefc1ed9ee2db7f66a9

But I'll close this PR for now and continue the discussion in #141.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for looking into it 🙇 That's a nice write-up for a tricky feature 👏


```bash
gateway-conformance test -skip 'TestGatewayCar/GET_response_for_application/vnd.ipld.car/Header_Content-Length'
gateway-conformance test --skip 'TestGatewayCar/GET_response_for_application/vnd.ipld.car/Header_Content-Length'
gateway-conformance test --skip 'TestGatewayCar/.*/vnd.ipld.car' --skip 'TestGatewayCar/GET_response_for_application/vnd.*'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

highlight: use in CLI

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @laurentsenta!

Details inline, quick feedback:

  • add support for passing multiple -run (for parity with -skip)
  • could we keep args in github action examples? (this way we don't need special casing for runs)

@@ -235,10 +237,11 @@ gateway-conformance test --specs trustless-gateway,-trustless-gateway-ipns

### Skip a specific test
Copy link
Member

@lidel lidel Aug 22, 2023

Choose a reason for hiding this comment

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

Could we add ### Run only a specific test with example that uses go's go test -run <pattern> and use the same pattern as in the skip example?

Include this note:

The -run argument uses the same syntax as go test -run and gives more fine-grained control than predefined presets passed via --specs. It aims to save time when developing a fix and having to re-run specific subset of tests without running the entire suite.

xml: output.xml
html: output.html
markdown: output.md
skips: '["TestTrustlessCarPathing", "TestNativeDag/GET_plain_JSON_codec_from_.*"]'
Copy link
Member

@lidel lidel Aug 22, 2023

Choose a reason for hiding this comment

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

Regex support is pretty recent, i think? https://tip.golang.org/doc/go1.20 states:

The go test command now accepts -skip to skip tests, subtests, or examples matching .

go help testflag conforms we now have parity for -run and -skip which is really nice!:

-run regexp
	    Run only those tests, examples, and fuzz tests matching the regular
	    expression. For tests, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any. Note that possible parents of matches are
	    run too, so that -run=X/Y matches and runs and reports the result
	    of all tests matching X, even those without sub-tests matching Y,
	    because it must run them to look for those sub-tests.
	    See also -skip.
[..]
	-skip regexp
	    Run only those tests, examples, fuzz tests, and benchmarks that
	    do not match the regular expression. Like for -run and -bench,
	    for tests and benchmarks, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any.

Both skip and run are useful. run allows more fine-grained control than our predefined presets, which saves time when someone develops a fix and has to re-run specific subset of tests without running the entire suite (not a problem today, but the suite will grow).

@laurentsenta do we need to have dedicated input in github action?
If we could keep this under args we would not have to do anything special to support -run here, right?

Comment on lines +118 to +124
&cli.StringSliceFlag{
Name: "skip",
Usage: "Accepts a test path to skip.\nCan be used multiple times.\n" +
"Example: --skip \"TestTar/GET_TAR_with_format=.*/Body\" --skip \"TestGatewayBlock\" --skip \"TestTrustlessCarPathing\"\n" +
"It uses the same syntax as the -skip flag of go test.",
Destination: &skips,
},
Copy link
Member

Choose a reason for hiding this comment

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

Parity with go-test -skip is 👍
Could we include the same for go-test -run in this PR?
It will make things feature-complete, and natural for anyone familiar with go, and at least remove surprises when someone tries -skip and -run – these should work the same, and if we support passing -skip multiple times, we should also support that for -run 🙏

@@ -235,10 +237,11 @@ gateway-conformance test --specs trustless-gateway,-trustless-gateway-ipns

### Skip a specific test

Tests are skipped using Go's standard syntax:
Tests are skipped using the `--skip` parameter and Go's standard syntax:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Tests are skipped using the `--skip` parameter and Go's standard syntax:
Tests can be skipped using the `--skip` parameter which follows `go test -skip regex` syntax, but can be passed more than once for convenience:

@galargh galargh removed their request for review September 28, 2023 11:22
@galargh
Copy link
Contributor

galargh commented Oct 12, 2023

@laurentsenta Should we close it?

@galargh
Copy link
Contributor

galargh commented Nov 14, 2023

Closing it now.

@galargh galargh closed this Nov 14, 2023
@galargh galargh deleted the feat/multiple-skips branch November 14, 2023 14:33
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.

Support -skip of multiple tests
3 participants