Skip to content
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

E2E test for every supported images #2402

Merged

Conversation

@prietyc123
Copy link
Collaborator

prietyc123 commented Nov 19, 2019

What kind of PR is this?

/kind test

What does does this PR do / why we need it:
Tests the e2e scenarios of supported images.
Which issue(s) this PR fixes:

Fixes #2364

How to test changes / Special notes to the reviewer:
make test-e2e-images

@openshift-ci-robot openshift-ci-robot requested review from amitkrout and kadel Nov 19, 2019
@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch 4 times, most recently from 800e731 to 660c0ff Nov 20, 2019
@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Nov 25, 2019

@kadel It seems prow does not handle the authentication token for registry.redhat.io for image openjdk-11-rhel8 .

I have created an upstream issue openshift/release#6056 to get an idea of fixing it more efficiently.

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from 660c0ff to 1626821 Nov 25, 2019
Copy link
Collaborator

amitkrout left a comment

Add the same e2e-image test target to travis configuration

@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Nov 25, 2019

Add the same e2e-image test target to travis configuration

Done

@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Nov 27, 2019

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from 5a13737 to c52e7cb Dec 4, 2019
@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Dec 4, 2019

@kadel It seems prow does not handle the authentication token for registry.redhat.io for image openjdk-11-rhel8 .

I have created an upstream issue openshift/release#6056 to get an idea of fixing it more efficiently.

Still looking into how to handle secret-id for registry.redhat.io for image openjdk-11-rhel8 .

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch 3 times, most recently from e7cda71 to 0e35ea8 Dec 5, 2019
@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Dec 11, 2019

/retest

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from 0e35ea8 to 87ea192 Dec 11, 2019
@amitkrout

This comment has been minimized.

Copy link
Collaborator

amitkrout commented Dec 12, 2019

/retest

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch 2 times, most recently from 3b797b9 to ecccc39 Dec 12, 2019
@prietyc123 prietyc123 changed the title [WIP]E2E test for every supported images E2E test for every supported images Dec 12, 2019
@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from 759d500 to fc7e7f8 Jan 17, 2020
@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Jan 17, 2020

@amitkrout can you please mount openshift/release#6341 in the job file

fc7e7f8

/hold

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from 4077e82 to ca1511c Feb 4, 2020
@girishramnani

This comment has been minimized.

Copy link
Collaborator

girishramnani commented Feb 4, 2020

I found something interesting about ginkgo that it supports table driven tests https://onsi.github.io/ginkgo/#table-driven-tests . So could you consider using that for these @prietyc123 .
cc @kadel

@prietyc123 prietyc123 force-pushed the prietyc123:E2ETestForEverySupportedImage branch from ca1511c to 59dbba0 Feb 5, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #2402 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2402      +/-   ##
==========================================
- Coverage   42.67%   42.59%   -0.09%     
==========================================
  Files          73       73              
  Lines        7062     7062              
==========================================
- Hits         3014     3008       -6     
- Misses       3762     3767       +5     
- Partials      286      287       +1
Impacted Files Coverage Δ
pkg/component/watch.go 70% <0%> (-4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8327ec...59dbba0. Read the comment docs.

@prietyc123

This comment has been minimized.

Copy link
Collaborator Author

prietyc123 commented Feb 5, 2020

/retest

@amitkrout

This comment has been minimized.

Copy link
Collaborator

amitkrout commented Feb 7, 2020

I found something interesting about ginkgo that it supports table driven tests https://onsi.github.io/ginkgo/#table-driven-tests . So could you consider using that for these @prietyc123 .
cc @kadel

@girishramnani Nice reference to go with tabular formatted test. However me and @prietyc123 had a quick discussion around it and found we have also few more places in out test script where tabular test format can be applied. For example config file test.

IMO it would be great if we create a code refactor issue against the test script and fix that in the later sprint. cc_ @prietyc123 @kadel

+1, Test script looks good to me.

@girishramnani

This comment has been minimized.

Copy link
Collaborator

girishramnani commented Feb 10, 2020

If the next step would be to use the tabular version then why not consider doing it as part of this PR?

@kadel

This comment has been minimized.

Copy link
Member

kadel commented Feb 10, 2020

How big change it would be to convert this into a tabular version?
If it is just a few hours it makes sense to do it here.

If it is a bigger refactor I would be ok with doing it in another PR together with bigger systematic change. As this PR is otherwise ready.

@amitkrout

This comment has been minimized.

Copy link
Collaborator

amitkrout commented Feb 11, 2020

If the next step would be to use the tabular version then why not consider doing it as part of this PR?

@girishramnani No atm because this is not only the place where we are going to do the change. Tabula format may affects more than one test files. I would say let's do the proper cost analysis i mean the total number of test files which are being affected and the time consume to implement the tabular format.

How big change it would be to convert this into a tabular version? If it is just a few hours it makes sense to do it here. If it is a bigger refactor I would be ok with doing it in another PR together with bigger systematic change. As this PR is otherwise ready.

@kadel Right now i am not sure, however me and @prietyc123 had a quick discuss on the tabular format and we found few places. May be tabular format affect more file than what we found in the quick discussion. So this need a proper analysis to find out files affected and cost.

I have created a separate issue to track the ginkgo tabular format implementation - #2590

@amitkrout

This comment has been minimized.

Copy link
Collaborator

amitkrout commented Feb 11, 2020

If the next step would be to use the tabular version then why not consider doing it as part of this PR?

@girishramnani No atm because this is not only the place where we are going to do the change. Tabula format may affects more than one test files. I would say let's do the proper cost analysis i mean the total number of test files which are being affected and the time consume to implement the tabular format.

How big change it would be to convert this into a tabular version? If it is just a few hours it makes sense to do it here. If it is a bigger refactor I would be ok with doing it in another PR together with bigger systematic change. As this PR is otherwise ready.

@kadel Right now i am not sure, however me and @prietyc123 had a quick discuss on the tabular format and we found few places. May be tabular format affect more file than what we found in the quick discussion. So this need a proper analysis to find out files affected and cost.

I have created a separate issue to track the ginkgo tabular format implementation - #2590

/lgtm

@girishramnani

This comment has been minimized.

Copy link
Collaborator

girishramnani commented Feb 12, 2020

/approve

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Feb 12, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 379a82a into openshift:master Feb 12, 2020
5 of 6 checks passed
5 of 6 checks passed
tide Not mergeable. Retesting: ci/prow/v4.3-integration-e2e-benchmark
Details
Travis CI - Pull Request Build Passed
Details
ci/prow/unit Job succeeded.
Details
ci/prow/v4.1-integration-e2e-benchmark Job succeeded.
Details
ci/prow/v4.2-integration-e2e-benchmark Job succeeded.
Details
ci/prow/v4.3-integration-e2e-benchmark Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.