Running spec parallelly for clean test template #1759

amitkrout · 2019-05-22T11:47:02Z

What is the purpose of this change? What does it change?

To reduce test time both locally and on CI too.

Was the change discussed in an issue?

fixes - part of #1473

How to test changes?

make clean-test

amitkrout · 2019-05-22T11:48:47Z

.travis.yml

@@ -34,6 +34,7 @@ jobs:
        - make bin
        - sudo cp odo /usr/bin
        - oc login -u developer
+        - travis_wait make clean-test


Keeping it only for testing purpose on travis. Will be removed before merge

amitkrout · 2019-05-22T11:49:05Z

scripts/openshiftci-presubmit-e2e.sh

@@ -11,6 +11,7 @@ make configure-installer-tests-cluster
 make bin
 export PATH="$PATH:$(pwd)"
 export CUSTOM_HOMEDIR="/tmp/artifacts"
+make clean-test


Keeping it only for testing purpose on prow. Will be removed before merge

amitkrout · 2019-05-22T11:50:24Z

/hold

amitkrout · 2019-05-23T12:08:49Z

/hold cancel

amitkrout · 2019-05-23T12:09:18Z

ping @mohammedzee1000 @dharmit PTAL

mohammedzee1000 · 2019-05-23T12:13:13Z

How are you installing ginko here?

amitkrout · 2019-05-23T12:27:28Z

How are you installing ginko here?

https://github.com/openshift/odo/pull/1759/files#diff-bbfcfec7ae4d7cff5d2b469274d9ccaaR13

mohammedzee1000 · 2019-05-23T13:02:49Z

/approve

openshift-ci-robot · 2019-05-23T13:03:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mohammedzee1000

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:

~~OWNERS~~ [mohammedzee1000]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-05-23T13:03:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mohammedzee1000

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:

~~OWNERS~~ [mohammedzee1000]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

girishramnani · 2019-05-24T06:04:08Z

a bit of documentation around SPEC_EXEC_METHOD? what does it do?

Also maybe a section in the our docs on running tests parallel or serially would be great @amitkrout - maybe a separate PR or in this itself

amitkrout · 2019-05-24T06:20:01Z

a bit of documentation around SPEC_EXEC_METHOD? what does it do?

Also maybe a section in the our docs on running tests parallel or serially would be great @amitkrout - maybe a separate PR or in this itself

Will add the doc change around SPEC_EXEC_METHOD in this. Reference of SPEC_EXEC_METHOD in doc helps to choose run style (parallel or series).

amitkrout · 2019-05-24T07:18:26Z

a bit of documentation around SPEC_EXEC_METHOD? what does it do?
Also maybe a section in the our docs on running tests parallel or serially would be great @amitkrout - maybe a separate PR or in this itself

Will add the doc change around SPEC_EXEC_METHOD in this. Reference of SPEC_EXEC_METHOD in doc helps to choose run style (parallel or series).

@girishramnani Doc updated. PTAL
Thanks @Preeticp for a quick guidance :)

cdrage

So confused with regards to this PR.

We remove the documentation about how to run e2e tests locally, but you provide documentation to use make clean-test...

Which doesn't even run all the e2e tests?

cdrage · 2019-05-24T16:29:07Z

docs/development.adoc

----
-
-* For the component tests use:
+* To run the test spec parallelly (default):


in parallel.

cdrage · 2019-05-24T16:40:32Z

docs/development.adoc

 ----
-$ make test-cmp-e2e
+$ make clean-test


why are we using "clean-test" this is fairly confusing, why not "e2e-test" ?

Here the intention is to provide the doc reference to @mohammedzee1000 and @dharmit to write other remaining test for parallel run.

cdrage · 2019-05-24T16:42:13Z

Makefile

+.PHONY: clean-test
+clean-test:
+ifeq ($(SPEC_EXEC_METHOD),series)
+	go test -v github.com/openshift/odo/tests/template --ginkgo.focus="Example of a clean test" -ginkgo.slowSpecThreshold=$(SLOW_SPEC_THRESHOLD) -ginkgo.v -timeout $(TIMEOUT)


I don't like that we use "Example of a clean test" it makes no sense. clean-test is running all e2e tests correct?

No, ```clean-test`` is just a template for reference of how to write clean and efficient integration/e2e test.

if it is just a template why do we need it in the makefile?

It is just for a reference for @mohammedzee1000 and @dharmit who are also going to pick some of the test. In the very next sub task of #1473.

Anyway i will add a note there so that we don't forget to remove it in the next subtask fix.

cdrage · 2019-05-24T16:44:56Z

docs/development.adoc

@@ -160,33 +160,22 @@ $ MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --extra-clusterup-flags "--ena

 .Procedure:

-To deploy an integration test:
+Integration tests can be run in the following two ways:


Yeah, the below information is completely wrong. You remove information on how to run the tests... (make test-e2e) and make clean-test doesn't even do anything.

You need to keep the make test-e2e here so that people can run these integration tests locally.

The documentation you're adding should be about running each test individually.

I have addressed those comment here #1759 (comment). Also i ahev added extra info in the NOTE. Probably the NOTE section will clarify most of your doubts.

cdrage · 2019-05-24T16:45:47Z

/hold
Please do not merge until the above comments are addressed ^^

amitkrout · 2019-06-11T06:51:46Z

@amitkrout

There is absolutely no documentation on your new command (make test-integration). You should add this to the doc: https://github.com/openshift/odo/blob/98ab997b375aa87cc37cd82320233b60ca080284/docs/development.adoc

In order to show how to run all the tests instead of just make test-cmp-e2e

Done, Currently most of the test make test-integration fails due to the pending work in #1473 and will be fixed one by one

kadel · 2019-06-11T14:58:54Z

Done, Currently most of the test make test-integration fails due to the pending work in #1473 and will be fixed one by one

Then we can't merge this PR until it is fixed. There are people that are using this, and you are breaking it.

amitkrout · 2019-06-12T06:55:06Z

Done, Currently most of the test make test-integration fails due to the pending work in #1473 and will be fixed one by one

Then we can't merge this PR until it is fixed. There are people that are using this, and you are breaking it.

Now i got the issue why it's taking long time to go in because i believe we are not in same page. My intention was to provide as much as info through doc or Makefile change to @mohammedzee1000 and @dharmit by pushing this into master first as discussed in our last sprint planning meeting. But it did not work that way.

Unassigned @mohammedzee1000 and @dharmit from issue #1473. i am going to take the full ownership of it.

@cdrage @kadel I have updated the pr. Can you pease have a look again.

kadel · 2019-06-12T09:38:36Z

Now i got the issue why it's taking long time to go in because i believe we are not in same page. My intention was to provide as much as info through doc or Makefile change to @mohammedzee1000 and @dharmit by pushing this into master first as discussed in our last sprint planning meeting. But it did not work that way.

The only issue is that you can't just add make targets that are not working that is all. If you want to provide info and guidelines do it via comments or documentation.

amitkrout · 2019-06-12T10:56:04Z

Now i got the issue why it's taking long time to go in because i believe we are not in same page. My intention was to provide as much as info through doc or Makefile change to @mohammedzee1000 and @dharmit by pushing this into master first as discussed in our last sprint planning meeting. But it did not work that way.

The only issue is that you can't just add make targets that are not working that is all. If you want to provide info and guidelines do it via comments or documentation.

Yup, i understand. I was just enforcing the the comment #1759 (comment) which obviously not a good idea. I have updated pr

Please do a review again

cdrage · 2019-06-12T12:54:49Z

Now i got the issue why it's taking long time to go in because i believe we are not in same page. My intention was to provide as much as info through doc or Makefile change to @mohammedzee1000 and @dharmit by pushing this into master first as discussed in our last sprint planning meeting. But it did not work that way.

The only issue is that you can't just add make targets that are not working that is all. If you want to provide info and guidelines do it via comments or documentation.

Yup, i understand. I was just enforcing the the comment #1759 (comment) which obviously not a good idea. I have updated pr

Please do a review again

That comment was intended to convey that we need documentation if you add a new Makefile command. It was since you removed the documentation on make test-e2e renamed it to make test-integration but didn't add any documentation about the command..

cdrage

changes LGTM.

/lgtm

amitkrout · 2019-06-12T13:23:56Z

/hold cancel

amitkrout · 2019-06-12T13:29:39Z

/refresh

openshift-ci-robot added the size/S label May 22, 2019

openshift-ci-robot requested review from kadel, mohammedzee1000 and girishramnani May 22, 2019 11:47

amitkrout commented May 22, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label May 22, 2019

amitkrout mentioned this pull request May 22, 2019

run e2e tests in parallel on OpenShift CI #1473

Closed

openshift-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. labels May 23, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 23, 2019

openshift-ci-robot added size/M and removed size/S labels May 24, 2019

cdrage suggested changes May 24, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label May 24, 2019

amitkrout added 11 commits June 11, 2019 11:50

Added TIMEOUT

321b091

Added GLOBALODOCONFIG for global config isolation

9ce7698

Fixed review comments

3fc64bf

Added test node env var and updated doc

beb9aa7

Added pwd in PATH

02148e6

Removed test template form Makefile

48c65e1

Updated doc

713af19

Documentation and description updated

fe082b9

Added ginkgo in travis CI

3d060da

Made ginkgo installation part of goget-tools

d86f667

Added doc for test-integration

72d334f

amitkrout force-pushed the testTemplateParallel branch from 98ab997 to 72d334f Compare June 11, 2019 06:47

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 11, 2019

Updated doc and Makefile

e596a11

openshift-ci-robot added size/S and removed size/M labels Jun 12, 2019

cdrage approved these changes Jun 12, 2019

View reviewed changes

openshift-ci-robot assigned cdrage Jun 12, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 12, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Jun 12, 2019

openshift-merge-robot merged commit 4eeaaf9 into redhat-developer:master Jun 12, 2019

rm3l added the estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person label Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Running spec parallelly for clean test template #1759

Running spec parallelly for clean test template #1759

amitkrout commented May 22, 2019

amitkrout May 22, 2019

amitkrout May 22, 2019

amitkrout commented May 22, 2019

amitkrout commented May 23, 2019

amitkrout commented May 23, 2019

mohammedzee1000 commented May 23, 2019

amitkrout commented May 23, 2019

mohammedzee1000 commented May 23, 2019

openshift-ci-robot commented May 23, 2019

openshift-ci-robot commented May 23, 2019

girishramnani commented May 24, 2019

amitkrout commented May 24, 2019

amitkrout commented May 24, 2019 •

edited

Loading

cdrage left a comment

cdrage May 24, 2019

amitkrout May 27, 2019

cdrage May 24, 2019

amitkrout May 27, 2019

cdrage May 24, 2019

amitkrout May 27, 2019

kadel May 29, 2019

amitkrout May 30, 2019

cdrage May 24, 2019

amitkrout May 27, 2019

cdrage commented May 24, 2019 •

edited

Loading

amitkrout commented Jun 11, 2019

kadel commented Jun 11, 2019 •

edited

Loading

amitkrout commented Jun 12, 2019

kadel commented Jun 12, 2019

amitkrout commented Jun 12, 2019

cdrage commented Jun 12, 2019

cdrage left a comment

amitkrout commented Jun 12, 2019

amitkrout commented Jun 12, 2019

Running spec parallelly for clean test template #1759

Running spec parallelly for clean test template #1759

Conversation

amitkrout commented May 22, 2019

What is the purpose of this change? What does it change?

Was the change discussed in an issue?

How to test changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout commented May 22, 2019

amitkrout commented May 23, 2019

amitkrout commented May 23, 2019

mohammedzee1000 commented May 23, 2019

amitkrout commented May 23, 2019

mohammedzee1000 commented May 23, 2019

openshift-ci-robot commented May 23, 2019

openshift-ci-robot commented May 23, 2019

girishramnani commented May 24, 2019

amitkrout commented May 24, 2019

amitkrout commented May 24, 2019 • edited Loading

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented May 24, 2019 • edited Loading

amitkrout commented Jun 11, 2019

kadel commented Jun 11, 2019 • edited Loading

amitkrout commented Jun 12, 2019

kadel commented Jun 12, 2019

amitkrout commented Jun 12, 2019

cdrage commented Jun 12, 2019

cdrage left a comment

Choose a reason for hiding this comment

amitkrout commented Jun 12, 2019

amitkrout commented Jun 12, 2019

amitkrout commented May 24, 2019 •

edited

Loading

cdrage commented May 24, 2019 •

edited

Loading

kadel commented Jun 11, 2019 •

edited

Loading