You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I think that would be simplest, quickest fix but it's not technically correct as test-v8-updates doesn't need to build node (the previous test-hash-seed did because it ran $(NODE) test/pummel/test-hash-seed.js).
Unfortunately the job was relying on the side-effect of Node.js being built by make test-v8 and subsequently directly runs $PYTHON tools/test.py.
# we run the `v8-updates` suite on machines labeled `*benchmark*`case"$nodes"in*benchmark*)
export$(grep PYTHON config.mk)
NODE_TEST_DIR=/home/iojs/node-tmp NODE_COMMON_PORT=15000 $PYTHON tools/test.py -j $JOBS -p tap --mode=release --flaky-tests=run v8-updates
# Flag test as unstable if `perf` is not found
perf --version ||exit 54
;;
esac
We could insert make all into that case statement, but then I'd worry that older release lines would end up building node twice.
I'd suggested going with your suggestion in that PR so we don't hold it up for this and iterate on it later if needed.
Sorry, my bad -- I mixed up test-v8-updates with test-v8. But actually adding all as a dependency for test-v8-updates is correct (as that does need a built node).
I'll update the job to try replacing directly running
Activity
richardlau commentedon Apr 2, 2025
It built in the previous build earlier today: https://ci.nodejs.org/job/node-test-commit-v8-linux/6478/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/
targos commentedon Apr 2, 2025
Seems to only happen with V8 13.4 (nodejs/node#57114)
richardlau commentedon Apr 2, 2025
@targos It's because of nodejs/node@fe6b2a5. Previously
allwas a Makefile dependency oftest-hash-seed(removed by that commit).targos commentedon Apr 2, 2025
Good find!
Would that be the correct fix?
richardlau commentedon Apr 2, 2025
I think that would be simplest, quickest fix but it's not technically correct as
test-v8-updatesdoesn't need to buildnode(the previoustest-hash-seeddid because it ran$(NODE) test/pummel/test-hash-seed.js).Unfortunately the job was relying on the side-effect of Node.js being built by
make test-v8and subsequently directly runs$PYTHON tools/test.py.We could insert
make allinto thatcasestatement, but then I'd worry that older release lines would end up buildingnodetwice.I'd suggested going with your suggestion in that PR so we don't hold it up for this and iterate on it later if needed.
targos commentedon Apr 2, 2025
the change doesn't fix it.
https://ci.nodejs.org/job/node-test-commit-v8-linux/6480/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/console
richardlau commentedon Apr 2, 2025
Sorry, my bad -- I mixed up
test-v8-updateswithtest-v8. But actually addingallas a dependency fortest-v8-updatesis correct (as that does need a builtnode).I'll update the job to try replacing directly running
with
and test runs with the V8 update PR and an earlier release staging branch.
richardlau commentedon Apr 2, 2025
Test run with nodejs/node@0114ac7 (from the update PR): https://ci.nodejs.org/job/node-test-commit-v8-linux/6481/
Test run with
v18.x-staging: https://ci.nodejs.org/job/node-test-commit-v8-linux/6482/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/richardlau commentedon Apr 4, 2025
I think this is resolved now. Please reopen if not.
targos commentedon Apr 4, 2025
Yes, thanks !