feat: multiple skips #148

laurentsenta · 2023-08-21T13:36:59Z

Closes #141

laurentsenta · 2023-08-21T13:37:42Z

.github/workflows/test-kubo-skipped-e2e.yml

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


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

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_.*)"'

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?

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.

👍 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.

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

laurentsenta · 2023-08-21T13:40:13Z

README.md


 ```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.*'


highlight: use in CLI

lidel

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)

lidel · 2023-08-22T11:17:42Z

README.md

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

 ### Skip a specific test


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.

lidel · 2023-08-22T11:19:16Z

.github/workflows/test-kubo-skipped-e2e.yml

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


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?

lidel · 2023-08-22T11:20:19Z

cmd/gateway-conformance/main.go

+					&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,
+					},


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 🙏

lidel · 2023-08-22T11:26:48Z

README.md

@@ -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:


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 · 2023-10-12T13:11:59Z

@laurentsenta Should we close it?

galargh · 2023-11-14T14:33:33Z

Closing it now.

feat: add a --skip parameter to skip one or more tests

e228e9e

laurentsenta commented Aug 21, 2023

View reviewed changes

laurentsenta force-pushed the feat/multiple-skips branch from 7d91bf3 to bee0406 Compare August 21, 2023 13:39

laurentsenta commented Aug 21, 2023

View reviewed changes

laurentsenta requested review from lidel and galargh August 21, 2023 13:40

laurentsenta force-pushed the feat/multiple-skips branch from bee0406 to 158f91f Compare August 21, 2023 13:41

laurentsenta added 2 commits August 21, 2023 15:48

feat: add docs and entrypoint to skips

038fb8f

ci: test skipped flag with kubo

c533802

laurentsenta force-pushed the feat/multiple-skips branch from 158f91f to c533802 Compare August 21, 2023 13:48

lidel requested changes Aug 22, 2023

View reviewed changes

lidel reviewed Aug 22, 2023

View reviewed changes

laurentsenta mentioned this pull request Aug 24, 2023

Support -skip of multiple tests #141

Open

galargh removed their request for review September 28, 2023 11:22

galargh closed this Nov 14, 2023

galargh deleted the feat/multiple-skips branch November 14, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multiple skips #148

feat: multiple skips #148

laurentsenta commented Aug 21, 2023

laurentsenta Aug 21, 2023

galargh Aug 21, 2023

lidel Aug 22, 2023 •

edited

Loading

galargh Aug 22, 2023

laurentsenta Aug 24, 2023

galargh Aug 24, 2023

laurentsenta Aug 21, 2023

lidel left a comment

lidel Aug 22, 2023 •

edited

Loading

lidel Aug 22, 2023 •

edited

Loading

lidel Aug 22, 2023

lidel Aug 22, 2023

galargh commented Oct 12, 2023

galargh commented Nov 14, 2023

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

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

		### Skip a specific test

	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:

feat: multiple skips #148

feat: multiple skips #148

Conversation

laurentsenta commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

lidel Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

lidel Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galargh commented Oct 12, 2023

galargh commented Nov 14, 2023

lidel Aug 22, 2023 •

edited

Loading

lidel Aug 22, 2023 •

edited

Loading

lidel Aug 22, 2023 •

edited

Loading