sclorg / container-common-scripts

Apache License 2.0
21 stars 45 forks source link

add support for running upstream testsuites #352

Closed zmiklank closed 7 months ago

zmiklank commented 8 months ago

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.

zmiklank commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Just one more note - the makefile target you are mentioning is currently suggested to be only in nodejs makefile, see https://github.com/sclorg/s2i-nodejs-container/pull/415

zmiklank commented 8 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@hhorak @pkubatrh WDYT?

phracek commented 7 months ago

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 commented 7 months ago

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

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

phracek commented 7 months ago

Please rebase it against the master

zmiklank commented 7 months ago

[test]