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

Add new xtf.openshift.namespace.per.testcase property which will exec… #490

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

mnovak1
Copy link
Contributor

@mnovak1 mnovak1 commented May 31, 2022

…ute every test case in its own namespace. Note this property has limitation that consuming test suite must not create static Openshift instances as those are created before name of test case and thus the namespace for the test is known.

Please make sure your PR meets the following requirements:

  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • Code is self-descriptive and/or documented

CI runs (no regression):

-Dxtf.openshift.namespace.per.testcase=false:
https://jenkins.eapqe.psi.redhat.com/job/eap-8.x-openshift-ts-face/251//artifact/output.html

-Dxtf.openshift.namespace.per.testcase=true:
https://jenkins.eapqe.psi.redhat.com/job/eap-8.x-openshift-ts-face/252//artifact/output.html

@mnovak1 mnovak1 requested review from mchoma and simkam May 31, 2022 12:30
README.md Outdated Show resolved Hide resolved
@mchoma
Copy link
Contributor

mchoma commented Jun 1, 2022

@mnovak1 Maybe I would appreciate some introduction how this is implemented. So I better understand it if this can be simplified even more. Overall idea seems OK to me and this seems on good way. But I feel code could be better organized to be even simpler. I drop some random thoughts, but maybe they are not relevant and I am missing something.

@mnovak1 mnovak1 force-pushed the try-parallel-runs branch 3 times, most recently from 7eafc5b to a540259 Compare June 2, 2022 12:17
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mnovak1 mnovak1 force-pushed the try-parallel-runs branch 2 times, most recently from a25c243 to 96723ba Compare June 9, 2022 13:56
README.md Outdated Show resolved Hide resolved
@mnovak1 mnovak1 force-pushed the try-parallel-runs branch 3 times, most recently from 645b25f to cd2cc5a Compare June 28, 2022 11:24
@mnovak1 mnovak1 force-pushed the try-parallel-runs branch 2 times, most recently from abdf9f1 to 7b066c5 Compare July 7, 2022 08:18
@mnovak1
Copy link
Contributor Author

mnovak1 commented Jul 7, 2022

@mchoma I've addressed all the last comments and added CI runs. Could you review it. please?

@mnovak1 mnovak1 force-pushed the try-parallel-runs branch 2 times, most recently from a709111 to e838233 Compare July 7, 2022 11:33
README.md Outdated Show resolved Hide resolved
@mchoma
Copy link
Contributor

mchoma commented Jul 8, 2022

Ok I run through changes and I do not have any signifacnt objection. But truth is I am looking for long time on MR. Maybe it would need some fresh view :).

Just to double check. For someone who is not interested in using xtf.openshift.namespace.per.testcase=true it has no migration consequences?

@mnovak1
Copy link
Contributor Author

mnovak1 commented Jul 11, 2022

Just to double check. For someone who is not interested in using xtf.openshift.namespace.per.testcase=true it has no migration consequences?

Besides use of the static OpenShift instance and use of NamespaceManager there is no migration issue.

@mchoma mchoma self-requested a review July 11, 2022 07:49
Copy link
Contributor

@mchoma mchoma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving.

…ute every test case in its own namespace. Note this property has limitation that consuming test suite must not create static Openshift instances as those are created before name of test case and thus the namespace for the test is known.
Copy link
Contributor

@simkam simkam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think that mapping should be done at a different level. TestCase to namespaced OpenShiftClient rather than TestCase to OpenShiftClient to namespace.

But there might be reasons to go this way.

Quite a lot of static fields might also prove to be problematic in the future.

Anyway, approving.

@mnovak1
Copy link
Contributor Author

mnovak1 commented Jul 12, 2022

Thanks, I'll merge it.

TestCase to namespaced OpenShiftClient rather than TestCase to OpenShiftClient to namespace.

It's more mapping TestCase -> namespace -> OpenShiftClient. I think we've discussed it somewhere already. Motivation is to cache OpenShiftClient as much as possible and making it optimal when this feature is ON or OFF

Mapping TestCase to namespaced OpenShiftClient would work well when this feature is ON. But if it's OFF then it's not desirable to create as many OpenShiftClients as there are TestCases as most likely there would be just 2 OpenShiftClient instances for master and admin.
Mapping TestCase -> namespace -> OpenShiftClient solves this problem as when its' OFF then all TestCases has the same namespace and thus namespace maps again just to 2 OpenShiftClients.

@mnovak1 mnovak1 merged commit b9e3f2f into xtf-cz:master Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants