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

Run Intl tests in V8 CI #39053

Closed
targos opened this issue Jun 16, 2021 · 4 comments
Closed

Run Intl tests in V8 CI #39053

targos opened this issue Jun 16, 2021 · 4 comments
Labels
test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Jun 16, 2021

Maybe that would prevent regressions like #39050

@targos targos added v8 engine Issues and PRs related to the V8 dependency. test Issues and PRs related to the tests. labels Jun 16, 2021
@richardlau
Copy link
Member

I added running make test-v8-intl to the CI job but that failed as we were still passing --mode to the test runner: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/4059/console

11:56:05 	deps/v8/tools/run-tests.py --gn --arch=x64 \
11:56:05 			--mode=release intl \
11:56:05 			--junitout /home/iojs/build/workspace/node-test-commit-v8-linux/v8-intl-tap.xml
11:56:05 Usage: run-tests.py [options] [tests]
11:56:05 
11:56:05 run-tests.py: error: no such option: --mode
11:56:05 Makefile:668: recipe for target 'test-v8-intl' failed

PR: #39055

@richardlau
Copy link
Member

richardlau commented Jun 16, 2021

Adding a second make target (test-v8-intl) caused a second V8 build (since the dependencyv8 target is .PHONY and therefore always run). I've changed the job instead to add intl to the end of V8_TEST_EXTRA_OPTIONS:

node/Makefile

Lines 661 to 663 in e4eadb2

deps/v8/tools/run-tests.py --gn --arch=$(V8_ARCH) $(V8_TEST_OPTIONS) \
mjsunit cctest debugger inspector message preparser \
$(TAP_V8)

I ran a build against v14.x-staging: https://ci.nodejs.org/job/node-test-commit-v8-linux/4063/
This appears to have run intl tests -- they all passed so it isn't catching #39050: https://ci.nodejs.org/job/node-test-commit-v8-linux/4063/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/testReport/(root)/v8tests/

@targos
Copy link
Member Author

targos commented Jun 20, 2021

they all passed so it isn't catching #39050

Yeah, I think I was wrong thinking that it could catch things like that make test-v8 doesn't use our version of ICU, but the one V8 directly depends on.

@targos
Copy link
Member Author

targos commented Jun 20, 2021

Anyway, thanks for making the changes!

@targos targos closed this as completed Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

2 participants