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 support for running upstream testsuites #352

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

zmiklank
Copy link
Contributor

The nodejs container needs to implement new test target, so that the client upstream testsuites can be run separately from other tests. This commit is for support of this new feature.

Setting as draft for now.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request.

The test called run-upstream should be also present here https://github.com/sclorg/container-common-scripts/blob/master/common.mk#L104
Similar like this one, otherwise the test will not be executed by CI.

@zmiklank
Copy link
Contributor Author

zmiklank commented Nov 13, 2023

Yeah, the current implementation does not allow to run nodejs upstream unit tests in our CI. We did not really specified if we want to run them there or not. This will require some discussion. From my POV it is not needed to run these tests in the CI. If it was, then we probably would not want to create different target for them. But as I already wrote, let's discuss this also with @hhorak and @pkubatrh.

@phracek
Copy link
Member

phracek commented Nov 13, 2023

eah, the current implementation does not allow to run nodejs upstream unit tests in our CI. We did not really specified if we want to run them there or not. This will require some discussion.

In case we would like to run these tests in Nightly Builds or even by PR, then we need target in Makefile.

@zmiklank
Copy link
Contributor Author

Just one more note - the makefile target you are mentioning is currently suggested to be only in nodejs makefile, see sclorg/s2i-nodejs-container#415

@zmiklank
Copy link
Contributor Author

eah, the current implementation does not allow to run nodejs upstream unit tests in our CI. We did not really specified if we want to run them there or not. This will require some discussion.

In case we would like to run these tests in Nightly Builds or even by PR, then we need target in Makefile.

Sure, but would we? Isn't the goal of this PR to move this workload to different CI, where the results would be relevant?

@phracek
Copy link
Member

phracek commented Nov 14, 2023

All our https://github.com/sclorg repositories use common.mk that is used in repo CI, NightlyBuilds, sanity checks and even GitLab CI, etc. It is easy to call just make test, make test-openshift, make test-openshift-4 and in this case make test-upstream. Also the other repos can have 'upstream' test suite, if it is feasible.

I don't follow, what different CI means. I guess, we will execute make test-upstream once per day or week by https://github.com/sclorg/ci-scripts/blob/master/daily_tests/daily_scl_tests.sh. This depends on NodeJS folks, what is their expectations.

Also s2i-python-container has unstable test https://github.com/sclorg/s2i-python-container/blob/master/test/run#L30. In case container-common-scripts provides common Makefile .PHONY target, then it can be used in this container as well.

To create a specific target in s2i-nodejs-container like you mentioned, excluding minimal and micro containers does not make sense from my point of view.

@phracek
Copy link
Member

phracek commented Nov 14, 2023

Based on the discussion with NodeJS team, there will be two possibilities, how to execute these tests.

  1. As NightlyBuilds but the mail will be send to @phracek and @lholmquist only. No others will be informed.
  2. By PR [test-upstream]. In case of the failure on the PR. We can merge it even if [test-upstream] is failing. This will not blocked our PR.

Hopefully nightly builds will be enough. From NightlyBuilds point of view, I need Makefile target as well and later on during a week I will incorporate these changes to NightlyBuild script.
@lholmquist What are prefered targets for these upstream tests all RHEL variants? Or even CentOS Streams.
No OpenShift tests will be run against upstream repos. Sorry, it does not make sense.

@zmiklank
Copy link
Contributor Author

All our https://github.com/sclorg repositories use common.mk that is used in repo CI, NightlyBuilds, sanity checks and even GitLab CI, etc. It is easy to call just make test, make test-openshift, make test-openshift-4 and in this case make test-upstream. Also the other repos can have 'upstream' test suite, if it is feasible.

Well that's actually a point worth discussion. For the future -- are there any other containers that will need separated target for upstream tests? I do not recall any -- thus it makes more sense to me to implement the makefile target only to this(nodejs) specific container image repo.. But if you have some in mind please state it.

I don't follow, what different CI means. I guess, we will execute make test-upstream once per day or week by https://github.com/sclorg/ci-scripts/blob/master/daily_tests/daily_scl_tests.sh. This depends on NodeJS folks, what is their expectations.

As I understood this feature, we wanted to introduce different target in order to not slow down our PR CI. If we wanted to run these tests in every CI then the UNSTABLE TESTS functionality could be just enough to cover the use-case.
So different CI in this case means CI that is not running under our PRs. It could be nightly testsuite, or some CI owned by the runtime team - the result will be for them anyway.

To create a specific target in s2i-nodejs-container like you mentioned, excluding minimal and micro containers does not make sense from my point of view.

Minimal and micro images have never run the upstream testsuites AFAIK.

@phracek
Copy link
Member

phracek commented Nov 16, 2023

@hhorak @pkubatrh WDYT?

@phracek
Copy link
Member

phracek commented Nov 16, 2023

Or may be mentioned in README.md that new make target is used in s2i-nodejs-container, so we know about it in the future.

But my main point is not to spread into *-container repos in the future alone targets. Let's define them in the one place and in case script does not exist, then do not execute the tests.

@zmiklank
Copy link
Contributor Author

Or may be mentioned in README.md that new make target is used in s2i-nodejs-container, so we know about it in the future.

But my main point is not to spread into *-container repos in the future alone targets. Let's define them in the one place and in case script does not exist, then do not execute the tests.

Ok, let's sum up the main dilemmas:

  1. make target in common.mk or in Makefile in s2i-nodejs
  • if we choose common.mk, the change would be more generic, but all repos except for nodejs will have new empty target
  1. run the upstream tests in PR CI or in nightly testsuite
  • either of these will require changes in tmt plans
  • if we choose PR CI, then I expect some need for changes in tfaga-wrapper (not good - this wrapper should be as generic as possible)
  1. [test-all] target in PR CI will run [test]&[test-openshift]&[test-upstream] OR [test]&[test-openshift]

I suggest to discuss these main dilemmas on Wednesday in meeting. WDYT?

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

CentOS Stream 9 is not supported here: https://github.com/sclorg/container-common-scripts/tree/master/tests/failures/check/v0 Please add this Dockerfile so tests are passing.

@phracek
Copy link
Member

phracek commented Nov 23, 2023

Please rebase it against the master

@zmiklank
Copy link
Contributor Author

[test]

@zmiklank zmiklank merged commit c5d8cea into sclorg:master Nov 23, 2023
2 checks passed
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.

2 participants