fix: use inferred supported features to set extendedSupportedFeatures #4113
Conversation
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
|
/hold |
conformance/utils/suite/suite.go
Outdated
| } | ||
| for _, f := range conformanceProfile.ExtendedFeatures.UnsortedList() { | ||
| if options.SupportedFeatures.Has(f) { | ||
| if suite.SupportedFeatures.Has(f) { |
There was a problem hiding this comment.
I think thats good, but need someone else's eyes on this to confirm.
There was a problem hiding this comment.
Can you describe and what's the changing here?
suite.SupportedFeatures is created from options.SupportedFeatures and when we are updating suite.SupoortedFeatures (which will be used for testing) we are checking if the proper features were populated in the options.SupportedFeatures, we don't want simultaneously check and update the same set, as it makes logic vulnerable to some obscure bugs.
P.S. also can you please add unit tests for the fixed logic.
There was a problem hiding this comment.
@bexxmodd the bug is described in #4112.
The issue occurs when supported features are inferred from the GatewayClass, in those cases, they aren’t included in options.SupportedFeatures but only set in suite.SupportedFeatures. As a result, the conformance test report incorrectly marks them as unsupported.
There was a problem hiding this comment.
Discussed this with @LiorLieberman offline. I see the issue.
Just a nit: Can you please use supportedFeatures for check, instead of suite.SupportedFeatures ? As in above comment, it's not a great practice to update the set that is also checked upon in the loop.
There was a problem hiding this comment.
But then we’d also have to change:
if !suite.SupportedFeatures.Has(f) {
suite.SupportedFeatures.Insert(f)
}to:
if !supportedFeatures.Has(f) {
suite.SupportedFeatures.Insert(f)
}That doesn’t seem right to me. I see supportedFeatures as a temporary variable that should only be used until the suite is initialized.
There was a problem hiding this comment.
For unit test coverage, I’d propose adding assertions for extendedSupportedFeatures in TestInferGWCSupportedFeatures.
There was a problem hiding this comment.
My bad. I meant to use supportedFeatures to check if it has a name or not.
There was a problem hiding this comment.
@bexxmodd I’m not sure I understand what you’re requesting, could you provide a code suggestion?
|
@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: bexxmodd. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@snorwin IIUC this only messes up reporting, right? I think this is LGTM from my side, will leave approval for someone else /lgtm |
| } | ||
| } | ||
| for _, f := range conformanceProfile.ExtendedFeatures.UnsortedList() { | ||
| if options.SupportedFeatures.Has(f) { |
There was a problem hiding this comment.
if !supportedFeatures.Has(f) {
suite.SupportedFeatures.Insert(f)
}
It's a temporary variable until final supported features are determined for testing and NewConformanceTestSuite returns ConformanceTestSuite. Nothing limits it's scope until suite is initialized. There's no scope conflict or a misleading name so I don't see why it should go out of usage after suite is initialized. it's the sound representation of populated features from the Status field.
There was a problem hiding this comment.
In my opinion, it doesn’t make sense to use supportedFeatures after it has already been assigned to suite.SupportedFeatures. Modifying it at that point would even be incorrect, why should we still rely on it?
In case additional code was inserted above this section that changed suite.SupportedFeatures, then the logic here is supposed to use the actual (possibly modified) features of the suite (suite.SupportedFeatures) rather than the originally derived ones in supportedFeatures.
If it is important that supportedFeatures is used in this part, I would suggest moving the entire section before the initialization of the ConformanceTestSuite object.
There was a problem hiding this comment.
It is actually a really good idea to move the whole section before initializing the ConformanceTestSuite.
There was a problem hiding this comment.
@bexxmodd could you take a close look at the refactoring? I simplified the logic quite a bit.
There was a problem hiding this comment.
This is way cleaner! Thanks Norwin!
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
c4e6820 to
68dd9e7
Compare
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
| } | ||
| } | ||
| for _, f := range conformanceProfile.ExtendedFeatures.UnsortedList() { | ||
| if options.SupportedFeatures.Has(f) { |
There was a problem hiding this comment.
This is way cleaner! Thanks Norwin!
|
sweet, thank you! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, LiorLieberman, snorwin 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 |
|
/unhold |
|
/cherry-pick release-1.4 |
|
@snorwin: new pull request created: #4118 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4112
Does this PR introduce a user-facing change?: