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

Removing node-test-commit-custom-suites-freestyle from node-test-commit #1864

Closed
rvagg opened this issue Jul 18, 2019 · 2 comments
Closed

Removing node-test-commit-custom-suites-freestyle from node-test-commit #1864

rvagg opened this issue Jul 18, 2019 · 2 comments
Labels

Comments

@rvagg
Copy link
Member

rvagg commented Jul 18, 2019

Since this is an unmaintained pipeline I'd like to explore rolling up whatever it contributes to node-test-commit into the containered builds if possible. I can't quite figure out what the purpose of this thing is and why it's so darn complicated.

From what I can glean from the bash and the way it's invokved, all it's doing is running the worker tests when invoked from node-test-commit.

It's passed CI_JS_SUITES=default and TEST_ARGS=--worker. The console output shows it doing:

  • make -j1 bench-addons-build
  • make -j 4 build-ci
  • python tools/test.py -j 4 -p tap --logfile test.tap --mode=release --flaky-tests=dontcare --worker

I think bench-addons-build is unnecessary for node-test-commit so it's really just a build-ci and the custom test.py, which is very similar to the way we invoke debug and withoutssl runs in the containered jobs (with a bonus that the configs are so much simpler).

It's also got if [ "$TEST_ARGS" = "--worker" ] && [ "$NODEJS_MAJOR_VERSION" -lt "10" ]; then so it's limited to Node >= 10. This should go into VersionSelectorScript.groovy (there's more of this logic for s390 and running v8-updates, those should be pulled out too).

So, I believe that if I pull out a --worker invocation of test.py into a containered job, put a >=10 restriction in VersionSelectorScript.groovy then I could remove this from node-test-commit. If there's anyone around that has more of a clue about this and believes this assessment is wrong, please speak up now. I'm really unhappy with this weight around our necks and would like it gone. It can be left as is for whatever nightly jobs its doing, but I also wouldn't mind removing those too in due course.

@richardlau
Copy link
Member

Since this is an unmaintained pipeline I'd like to explore rolling up whatever it contributes to node-test-commit into the containered builds if possible. I can't quite figure out what the purpose of this thing is and why it's so darn complicated.

It's complicated because it has been (ab)used in different contexts beyond its original purpose. The job was originally written to allow one-off testing of things not tested by the regular CI runs (e.g. want to run the internet or pummel test suites? Or run without intl (this was before the containerized jobs)). Somewhere along the line as requests came in to test more things as part of the regular/nightly CI and instead of adding new jobs the custom-suites jobs was just chained into the other jobs called with the appropriate parameters:
e.g.:

I think the job is useful for its original purpose of one-off or PR specific tests (for example nodejs/node#28733 (comment)). For things in the regular/nightly CI runs I think it would be much clearer to have dedicated jobs (:+1: for containered where it makes sense).

@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants