-
Notifications
You must be signed in to change notification settings - Fork 244
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
Run all integration test in parallel #1912
Run all integration test in parallel #1912
Conversation
3e1ade5
to
d763b3c
Compare
a2be939
to
10f57c6
Compare
fbc994e
to
29967ab
Compare
51f0fa9
to
a434986
Compare
/test integration |
/test integration |
/test integration |
/test integration |
/test integration |
Tested on CI (OpenShift [1] and travis[2]), however found a flake #1955 on OpenShift CI. [1] https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/1912/pull-ci-openshift-odo-master-integration/1033 |
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.
- Requested few minor changes with the code documentation
- At multiple places, I see that
project
variable's definition is being moved by a few lines. Since it's happening many times in the PR, I'm curious to know it's significance.
Makefile
Outdated
.PHONY: test-integration | ||
test-integration: | ||
go test -v github.com/openshift/odo/tests/integration -ginkgo.slowSpecThreshold=$(SLOW_SPEC_THRESHOLD) -ginkgo.v -timeout $(TIMEOUT) | ||
# Runs all integration test irrespective of service catalog status in the cluster |
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.
When we say "Runs all integration tests" it includes every test, isn't it? But link and service catalog tests run through test-integration-all-service-catalog
.
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.
yup, i understand.
I am open to better name suggestion (Keep in mind that tests depending on service catalog are not part of OpenShift CI).
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 not suggesting a name change. I'm suggesting to mention that "link" and "service catalog" tests do not run as a part of this test. 🙂
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.
Done, does it make sense ?
Makefile
Outdated
-focus="odoCmdApp|odoCmpSubE2e|odoCmpE2e|odo config test|odo storage command|odoWatchE2e|odo generic|odoJavaE2e|odojsonoutput|odo push command tests|odoURLIntegration|odoSourceE2e" \ | ||
slowSpecThreshold=$(SLOW_SPEC_THRESHOLD) -randomizeAllSpecs tests/integration/ -timeout 7200s | ||
|
||
# Run all integration test which are depend on service catalog enabled cluster |
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.
In this comment, can we mention the tests that are dependent on service catalog being enabled in cluster? And, if relevant, also mention that in future, tests which require service catalog should be added under this PHONY and not elsewhere?
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.
In this comment, can we mention the tests that are dependent on service catalog being enabled in cluster?
Agree
And, if relevant, also mention that in future, tests which require service catalog should be added under this PHONY and not elsewhere?
This piece of info i am going to add as part of #1831
* For the entire integration test suite: | ||
Entire integration tests suite can be run in parallel and sequentially separately: | ||
|
||
* To run entire integration tests spec in parallel (default: 2 ginkgo test node), on a test cluster: |
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.
Should we also mention how someone can set the value to something other than 2 if it's relevant for their environment?
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.
Sure, i can put a note or a place where it fits better in the doc
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.
Done
docs/development.adoc
Outdated
---- | ||
+ | ||
|
||
NOTE: `make test-integration` doesn't honour enviornment variable `TEST_EXEC_NODES`. So by default it runs the entire integration test suite on a single ginkgo test node sequentially. | ||
NOTE: `make test-integration-all-service-catalog` can be successfully validated only if the cluster has service catalog enabled. Whereas `make test-integration-all` runs the test irrespective of service catalog status in the cluster. |
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.
But, IIUC, make test-integration-all
doesn't run the tests which explicitly require service catalog to be up. Should we mention that somewhere in this line or elsewhere? Apologies if my understanding is incorrect or it has been mentioned elsewhere already.
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.
Should we mention that somewhere in this line or elsewhere?
I am sorry, could not get you.
Whereas `make test-integration-all` runs the test irrespective of service catalog status in the cluster.
This is the explanation what you are looking for i guess. Right ?
@@ -115,7 +115,7 @@ func NewPreferenceInfo() (*PreferenceInfo, error) { | |||
configFile, err := getPreferenceFile() | |||
glog.V(4).Infof("The configFile is %+v", configFile) | |||
if err != nil { | |||
return nil, errors.Wrap(err, "unable to get odo config file") | |||
return nil, errors.Wrap(err, "unable to get odo preference file") |
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.
Shouldn't we also change https://github.com/openshift/odo/pull/1912/files#diff-5f21db40ee2e66f00f206912f4dddc2fR116 to write preferenceFile
instead of configFile
? Using configFile
and preferenceFile
interchangeably could cause ambiguity, no?
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.
+1, I am going to update it in the next commit.
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.
Done
context = helper.CreateNewContext() | ||
os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml")) | ||
project = helper.CreateRandProject() |
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.
Out of curiosity, what impact does this have whether we create a project earlier or later?
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.
It is just to make sure that the set global preference path is used in the project creation step though there is no validation done for it. But if you keep the project create step as it was then project creation step picks the default preference path (~/.odo/preference.yaml) and then the very next odo command start picking the preference path from newly set path.
So i cleaned up this in all test file and later i will create some fresh spec where the validation will be done including all possible cases without duplicating the scenario.
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.
This is for another discussion, but I think that CreateRandProject
should be using oc
not odo
.
c4938ce
to
65e2e04
Compare
b56eccb
to
36fd8f4
Compare
/lgtm |
What is the purpose of this change? What does it change?
Runs all command integration test through a single .PHONY statement.
Was the change discussed in an issue?
fixes - part of #1473
How to test changes?
make test-integration-all
make test-integration-all-service-catalog