Fix compiler errors when viewing e2e tests in the IDE's #2130
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
area/provider/azure
jsturtevant
force-pushed
the
fix-e2e-build-errors
branch
from
cfbbf6c to
50a5172
Compare
|
I think the |
|
@jsturtevant I'm excited for this to get fixed! Would this fix all the compiler errors in |
|
|
|
||
| // validateStableReleaseString validates the string format that declares "get be the latest stable release for this <Major>.<Minor>" | ||
| // it should be called wherever we process a stable version string expression like "stable-1.22" | ||
| func validateStableReleaseString(stableVersion string) (bool, []string) { |
There was a problem hiding this comment.
why does this one need to move and not the other funcs above?
There was a problem hiding this comment.
it depends if the function is used outside one of the _test.go files. In this case this function is by aks.go:
cluster-api-provider-azure/test/e2e/aks.go
Line 268 in fb0a7f4
because the variable is defined in the _test.go the IDE compilers don't find it.
I do find it interesting that when running go test that the variables defined in a _test.go are found by the standard .go files (which is why this doesn't through error when we run it in out suite. 🤔
There was a problem hiding this comment.
we should probably move the other functions above for resolving k8s versions to helpers anyways since they could technically be reused by other files, even if we're not using them now, wdyt?
test/e2e/e2e_suite.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package e2e |
There was a problem hiding this comment.
I wonder if something like "e2e_helpers" or "e2e_utils" would be a more appropriate name for this file
There was a problem hiding this comment.
I would actually question why we need a new file at all. Between the existing test/e2e/e2e_suite_test.go and test/e2e/helpers.go do we not have the appropriate places for these additions?
There was a problem hiding this comment.
oh nevermind it's literally the thing that solves the stated problem
There was a problem hiding this comment.
I could move it to test/e2e/helpers.go I think. the comments
validateStableReleaseString should probably go to helpers.go. wdyt?
There was a problem hiding this comment.
I just find it confusing because to me e2e_suite.go implies there is a test suite in the file but it's just variables and constants. What about naming it e2e_suite_vars.go or something like that?
Side note, does CAPI have the same issue? I've noticed they also have test suite flags and vars in the main e2e_suite_test.go file
There was a problem hiding this comment.
What about naming it
e2e_suite_vars.goor something like that?
sounds good.
Side note, does CAPI have the same issue? I've noticed they also have test suite flags and vars in the main
e2e_suite_test.gofile
Just looked. It seems they have a slightly different set up. Where the files that reference variables that these variables always use _test.go and they have another file that live along side it. For example:
https://github.com/kubernetes-sigs/cluster-api/blob/main/test/e2e/cluster_upgrade.go
https://github.com/kubernetes-sigs/cluster-api/blob/main/test/e2e/cluster_upgrade_test.go
jsturtevant
force-pushed
the
fix-e2e-build-errors
branch
2 times, most recently
from
9f9c9be to
314c9f2
Compare
jsturtevant
force-pushed
the
fix-e2e-build-errors
branch
from
314c9f2 to
cddeed9
Compare
|
/test ls |
|
@jsturtevant: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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/test-infra repository. |
|
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts |
jsturtevant
force-pushed
the
fix-e2e-build-errors
branch
from
cddeed9 to
4095e5b
Compare
|
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts |
|
/assign @CecileRobertMichon @Jont828 apidiff job failure is due to the fact that some of the files didn't have the e2e build flag |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
I don't believe the time out is due to the pr |
|
@jsturtevant: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
When viewing the e2e suite in vscode or golang it throughs compilier errors which prevent functionality like goto or find references. This moves the variables from
_testto a non test file which allows it to build properly. This also allows for some additional entry points to be created (I've created one for gmsa which makes testing faster).This also added the 1.17 build tags to the files missing it in this directory.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: