feat: turn entrypoint into gateway-conformance CLI #15

galargh · 2023-03-16T16:43:14Z

I extracted file renames/moves to a separate commit so reviewing commit by commit might make the most sense: c67b7df?diff=split&w=1

Please note that the last commit introduces a different approach to test flags: 74b0079

- add support for go test args to the entrypoint - update Go version to 1.20 - rename is-subdomain to subdomain-gateway-spec - add merged as argument to the extract-fixtures command - rename entrypoint as gateway-conformance - rename tests package from main to tests - rename build tag to no_subdomain_gateway_spec - add a utility function for accessing GATEWAY_CONFORMANCE_HOME

galargh · 2023-03-20T10:20:45Z

tooling/specs/specs.go

+const (
+	SubdomainGateway Spec = "subdomain-gateway"
+)


Maybe these could correspond to the keywords/tag that Robin told us about.

galargh · 2023-03-20T10:22:05Z

tooling/specs/specs.go

+	wip        maturity = "wip"
+	draft      maturity = "draft"
+	reliable   maturity = "reliable"
+	stable     maturity = "stable"
+	permanent  maturity = "permanent"
+	deprecated maturity = "deprecated"


The updated specs specify maturity level per spec: https://github.com/ipfs/specs/pull/382/files#diff-68949a04625be3a1b31a438f35a6e8daaeb881ffd4a1631e083b3b070222e2f9

galargh · 2023-03-20T10:22:34Z

tooling/specs/specs.go

+func (s Spec) IsEnabled() bool {
+	if enabled, ok := specEnabled[s]; ok {
+		return enabled
+	} else {
+		return s.IsMature()
+	}
+}


If a spec wasn't enabled/disabled explicitly, we infer whether it's enabled or not based on its' maturity.

galargh · 2023-03-20T10:25:55Z

tests/init.go

+		if err != nil {
+			return err
+		}
+		if strings.HasPrefix(name, "+") {


If a spec is prefixed with +, it should be enabled. This can be used to enable tests for specs that are disabled by default (those not mature enough).

For example, let's say spec fancy has maturity wip. If we run tests without any flags, then tests for fancy would be skipped. If we used -specs=+fancy, tests for fancy would be executed AND all the tests that are enabled by default would be executed too.

Should we keep these comments somewhere in the README?

galargh · 2023-03-20T10:27:13Z

tests/init.go

+		}
+		if strings.HasPrefix(name, "+") {
+			enable = append(enable, spec)
+		} else if strings.HasPrefix(name, "-") {


If a spec is prefixed with -, it should be disabled. This can be used to disable tests for specs that are enabled by default (those mature enough).

For example, we can disable subdomain-gateway tests while still testing all the other specs that are enabled by default by providing -specs=-subdomain-gateway flag.

galargh · 2023-03-20T10:28:29Z

tests/init.go

+			enable = append(enable, spec)
+		} else if strings.HasPrefix(name, "-") {
+			disable = append(disable, spec)
+		} else {


If a spec is not prefixed with + nor -, we assume that the user wants to execute only that spec. Using flag -specs=subdomain-gateway would disable ALL the specs and then enable subdomain-gateway only.

galargh · 2023-03-20T10:29:54Z

tests/init.go

+var specsFlagValue specsFlag
+
+func init() {
+	flag.Var(&specsFlagValue, "specs", "comma-separated list of specs to test")


I implemented this as a single -specs flag with differing behaviour based on prefixes but it might as well be three separate flags -only, -enable, -disable. I like the conciseness of -specs personally but I definitely can see an argument being made for verbosity of breaking it up.

galargh · 2023-03-20T10:33:34Z

cmd/gateway-conformance/main.go

+					&cli.StringFlag{
+						Name:        "specs",
+						Usage:       "A comma-separated list of specs to test",
+						Value:       "",
+						Destination: &specs,


I thought about it and I think I'd prefer an interface that people can get used to. If we have separate flag for each spec (as it was here before), then we'd have to change that interface every time a new spec is added to the tests. I think it'd become quite overwhelming with the amount of different specs we might want to support. Also, it'd be harder to maintain as we'd have to remember to update the lists in all these places. In my opinion a single (or three) flags that take a list of specs will be easier to manage in a long run.

It might be useful to add a command for listing all the specs!

galargh · 2023-03-20T10:38:43Z

tests/t0114_gateway_subdomains_test.go

-//go:build !no_subdomain_gateway_spec
-// +build !no_subdomain_gateway_spec
-


Build tags are cool but I think they might not be powerful enough. E.g. if we had tests that are intersectional, then we'd have to create a new file for them. What I'm saying is, we could probably still achieve similar results but it'd be harder than if we used something that's available during runtime. The additional benefit is that with runtime exclusion, we can choose for the skipped tests to show up as skipped in the report. If we don't build a file, it's as if the tests never existed.

galargh · 2023-03-20T10:42:05Z

tests/t0114_gateway_subdomains_test.go

+	if SubdomainGateway.IsEnabled() {
+		Run(t, tests)
+	}


I added it like this for now for simplicity. What I imagine in the future is that each test case comes with a list of specs it tests. Before running the test case, we check if all the specs are enabled. If not, we skip that test case. Additionally, if a test case fails, we could link to the spec in the error message.

To not have to specify specs for each test case, we could wrap the encapsulating array in some class and store the defaults there.

👍 we can piggy back on the SugarTest I'm working on in

gateway-conformance/tests/t0118_gateway_car_test.go

Line 13 in c9cd6ca

tests := SugarTests{

,
we can create a follow up task for this

galargh · 2023-03-20T10:44:07Z

.github/workflows/test.yml

@@ -34,7 +34,7 @@ jobs:
          xml: output.xml
          html: output.html
          markdown: output.md
-          subdomain-gateway-spec: false
+          specs: -subdomain-gateway
          args: -skip TestGatewayCar


Disabled it by test name here. I think it's still useful in a case like this. Kubo does support CARs so it wouldn't make sense to disable testing CAR spec altogether. All we want is to disable this single test because Kubo's implementation is wrong at the moment.

laurentsenta

👍 (needs doc)

galargh mentioned this pull request Mar 16, 2023

feat: add entrypoint #14

Merged

galargh force-pushed the feat/gateway-conformance-cli branch 11 times, most recently from 1e6c3f2 to aaa57f9 Compare March 17, 2023 19:00

galargh added 3 commits March 17, 2023 20:09

chore: rename entrypoint.go and merge_fixtures.go

5565c04

chore: update files after the rename

591fd9e

galargh force-pushed the feat/gateway-conformance-cli branch from aaa57f9 to 591fd9e Compare March 17, 2023 19:10

feat: implement --specs=([+-]?(?P<name>[^,]+)[,|$])+

74b0079

galargh force-pushed the feat/gateway-conformance-cli branch from 10dc8f2 to 74b0079 Compare March 19, 2023 13:36

galargh commented Mar 20, 2023

View reviewed changes

galargh changed the title ~~wip: turn entrypoint into gateway-conformance CLI~~ feat: turn entrypoint into gateway-conformance CLI Mar 20, 2023

galargh requested a review from laurentsenta March 20, 2023 10:48

galargh marked this pull request as ready for review March 20, 2023 10:48

laurentsenta approved these changes Mar 20, 2023

View reviewed changes

laurentsenta mentioned this pull request Mar 20, 2023

Version 1.0 - Tasks #8

Closed

29 tasks

galargh added 2 commits March 21, 2023 10:00

docs: update the README

9be6d81

docs: update inline documentation

16b8ab0

galargh merged commit 5e12216 into feat/entrypoint Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: turn entrypoint into gateway-conformance CLI #15

feat: turn entrypoint into gateway-conformance CLI #15

galargh commented Mar 16, 2023 •

edited

Loading

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

laurentsenta Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

galargh Mar 20, 2023

laurentsenta Mar 20, 2023

galargh Mar 20, 2023

laurentsenta left a comment

		//go:build !no_subdomain_gateway_spec
		// +build !no_subdomain_gateway_spec

feat: turn entrypoint into gateway-conformance CLI #15

feat: turn entrypoint into gateway-conformance CLI #15

Conversation

galargh commented Mar 16, 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

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

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

laurentsenta left a comment

Choose a reason for hiding this comment

galargh commented Mar 16, 2023 •

edited

Loading