-
Notifications
You must be signed in to change notification settings - Fork 472
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
Add support for end-users to create their own conformance profiles #2864
Conversation
Also if we don't like this direction then I think it would be good to keep some of the refactor then it would be possible for downstream to run the test as follows func TestConformance(t *testing.T) {
opts := conformance.DefaultOptions()
opts.SupportedFeatures.Union(.... /* features I care about */)
conformance.RunConformance(t, opts)
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for adding this. A few comments, all minor.
5cb4376
to
eeda0fe
Compare
Few of the comments mentioned dropping the legacy support in this PR. I suggest we do this in a follow up PR. It'll be easier to understand the changes that took place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/cc @youngnick @robscott @mlavacca for lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold |
cea70b4
to
3b67c89
Compare
@mlavacca take a look now |
This lets downstreams to have their own tests with embedded yaml files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, mlavacca, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -199,10 +199,6 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite, | |||
mode = options.Mode | |||
} | |||
|
|||
if options.FS == nil { | |||
options.FS = &conformance.Manifests | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why you removed this part, but it's a breaking change for downstream projects.
If the downstream project uses gwapi's conformance test as a library, it will cause panic after this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the defaulting of the options here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dprotaso I know, but if downstream project not use DefaultOptions
, it will cause panic. like this one https://github.com/Kong/kubernetes-ingress-controller/actions/runs/8661628250/job/23752106390?pr=5863#step:5:290
--- FAIL: TestGatewayConformance (10.89s)
--- FAIL: TestGatewayConformance/GatewayInvalidRouteKind (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12be5d0]
goroutine 1479 [running]:
testing.tRunner.func1.2({0x452c460, 0x64d1600})
/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1545 +0x3f7
testing.tRunner.func1()
/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1548 +0x716
panic({0x452c460?, 0x64d1600?})
/opt/hostedtoolcache/go/1.21.6/x64/src/runtime/panic.go:920 +0x270
bytes.(*Buffer).Read(0x0, {0xc002d5a000, 0x1000, 0x7538b00?})
/opt/hostedtoolcache/go/1.21.6/x64/src/bytes/buffer.go:319 +0x30
bufio.(*Reader).fill(0xc001565ec0)
/opt/hostedtoolcache/go/1.21.6/x64/src/bufio/bufio.go:113 +0x29a
bufio.(*Reader).Peek(0xc001565ec0, 0x1000)
/opt/hostedtoolcache/go/1.21.6/x64/src/bufio/bufio.go:151 +0xc7
k8s.io/apimachinery/pkg/util/yaml.GuessJSONStream({0x50173a0?, 0x0}, 0x1000)
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/yaml/decoder.go:374 +0x1cb
k8s.io/apimachinery/pkg/util/yaml.(*YAMLOrJSONDecoder).Decode(0xc002d63b10, {0x48a9ca0, 0xc002ba6cc0})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/yaml/decoder.go:278 +0xae
sigs.k8s.io/gateway-api/conformance/utils/kubernetes.Applier.prepareResources({0x0, 0x0, {0xc0014b6570, 0x24}, {0xc000bb5080, 0x21}, {0x0, 0x0, 0x0}, {0x0, ...}, ...}, ...)
/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/kubernetes/apply.go:190 +0xc5
sigs.k8s.io/gateway-api/conformance/utils/kubernetes.Applier.MustApplyWithCleanup({0x0, 0x0, {0xc0014b6570, 0x24}, {0xc000bb5080, 0x21}, {0x0, 0x0, 0x0}, {0x0, ...}, ...}, ...)
/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/kubernetes/apply.go:254 +0x2a8
sigs.k8s.io/gateway-api/conformance/utils/suite.(*ConformanceTest).Run(0xc001565a40, 0xc001c4fd40, 0xc000eae000)
/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/suite/conformance.go:64 +0x706
sigs.k8s.io/gateway-api/conformance/utils/suite.(*ConformanceTestSuite).Run.func1(0x0?)
/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/suite/suite.go:390 +0x45
testing.tRunner(0xc001c4fd40, 0xc001404ee8)
/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1595 +0x262
created by testing.(*T).Run in goroutine 372
/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1648 +0x846
FAIL github.com/kong/kubernetes-ingress-controller/v3/test/conformance 86.039s
This is the configuration it uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tao12345666333 on this one. We cannot expect that implementation always use the DefaultOptions
method, as the suite is often used as a library. And in case FS is not set I think we should provide the default instead of expecting implementations to always set the default.
What type of PR is this?
/kind test
/kind cleanup
/area conformance
What this PR does / why we need it:
Advanced end users would like to create their own conformance profiles out of tree. This makes it easier to validate an implementation supports a certain set of features without relying on manually reading conformance reports on the gateway API website.
With this change writing your own profile to run is as simple as creating a single
_test.go
fileWhich issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: