-
Notifications
You must be signed in to change notification settings - Fork 127
Add wrapper which will allow running o/k tests as external binary in origin #1485
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
Conversation
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/hold |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/hold cancel |
|
/remove-label backports/unvalidated-commits |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/retest |
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.
Which functions/commit are you referring to?
$ grep -rn "initializeTestFramework"
cmd/openshift-tests/e2e.go:529: return initializeTestFramework(exutil.TestContext, opt.config, opt.DryRun)
cmd/openshift-tests/upgrade.go:85: if err := initializeTestFramework(exutil.TestContext, config, testOpt.DryRun); err != nil {
cmd/openshift-tests/openshift-tests.go:487: if err := initializeTestFramework(exutil.TestContext, config, testOpt.DryRun); err != nil {
cmd/openshift-tests/provider.go:36:func initializeTestFramework(context *e2e.TestContextType, config *exutilcluster.ClusterConfiguration, dryRun bool) error {
$ git log -1
commit bfcc30fde82a91fab1c881a836e4ad8e63c72fb5 (HEAD -> master, origin/release-4.15, origin/release-4.14, origin/master, origin/HEAD)
Merge: e63252bcd5 69f130654a
Author: OpenShift Merge Robot <[email protected]>
Date: Thu Apr 6 03:59:44 2023 -0400
Merge pull request #27842 from ardaguclu/oc-debug-pod-name-generation
not rely on deterministic pod names in oc debug testsThere 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.
This was about these 2 files:
- https://github.com/openshift/origin/blob/master/cmd/openshift-tests/provider.go)
- https://github.com/openshift/origin/blob/master/test/extended/util/test.go
Fixed the comment.
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.
What's the purpose of unmarshaling into &providerInfo? I.e. reference to providerInfo instead of just providerInfo?
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.
providerInfo in origin is populated here: https://github.com/openshift/origin/blob/bfcc30fde82a91fab1c881a836e4ad8e63c72fb5/cmd/openshift-tests/provider.go#L78, so it's done on every test invocation.
tl;dr
When you invoke openshift-tests run openshift/conformance/parallel it creates a list of tests and runs one by one invoking openshift-tests run-test. The above decodeProvider is responsible to read what the cluster looks like during that run-test invocation. Since we don't want to rely on that mechanism in here, we need to inject that value through env variable, and then decode it and use it.
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'm updating openshift/enhancements#1291 I'll add more details how this works there.
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.
What if initializeTestFramework gets invoked multiple times?. Invoking testfiles.AddFileSource does not implement any test for duplicates.
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.
k8s-tests run-test is invoked once per tests, see my explanation above, k8s-tests run-test is identical to openshift-tests run-test it only run one test only.
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 was thinking of a case when in the future initializeTestFramework gets accidentally invoked twice.
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.
What if there are multiple reports with report.NumAttempts > 0?
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.
That's a copy of the mechanism as was in origin, but that's a good question, I'll make sure to look more into it and fix it.
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.
Worth defining a function that will construct a name from the test name and the labels. In case the same naming constructions is (or will be) used some place else.
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.
Probably, but origin will directly use the annotation mechanism, without modifying it.
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.
The same here. Worth introducing a named function which will combine the test name and the labels into a string.
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.
Same as above.
openshift-hack/e2e/annotate/rules.go
Outdated
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.
These are new tests? Or, the tests are expected to be prefixed with [sig-...] from now on to be disabled? Or, these will appear after the 1.27 rebase lands?
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.
Those tests and rules are copied from origin, see Move k8s-specific rules to our fork commit in origin PR, previously we had a mix of rules in here and in origin. This PR splits the rules so they live where the test is.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
@soltysh: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/hold cancel |
|
/test e2e-aws-ovn-crun |
|
/test e2e-aws-ovn-serial |
|
/test e2e-azure-ovn-upgrade |
1 similar comment
|
/test e2e-azure-ovn-upgrade |
|
@soltysh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/test e2e-azure-ovn-upgrade |
This goes in pair with openshift/origin#27570
Write-up in openshift/enhancements#1291