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

build: convert V8 test JSON to JUnit XML #44049

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented Jul 30, 2022

This introduces some code to convert from V8's test JSON output to JUnit
XML. We need this because V8's latest refactor of their test runner has
made it difficult to float our JUnit reporter patch on top (see the
referenced issue).

I also think that there needs to be the same changes to vcbuild.bat, but
I don't know how to do/test those yet. I can create a Windows VM and
test it if we decide to go with this approach.

Closes: nodejs/node-v8#236

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Jul 30, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jul 30, 2022

/cc @nodejs/v8

Makefile Show resolved Hide resolved
tools/v8-json-to-junit.py Show resolved Hide resolved
@bnoordhuis bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Aug 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis
Copy link
Member

@kvakil Can you try this patch?

diff --git a/tools/test-v8.bat b/tools/test-v8.bat
index d322c31a38d..64265157f00 100644
--- a/tools/test-v8.bat
+++ b/tools/test-v8.bat
@@ -20,18 +20,21 @@ if errorlevel 1 set ERROR_STATUS=1&goto test-v8-exit
 set path=%savedpath%
 
 if not defined test_v8 goto test-v8-intl
-echo running 'python tools\run-tests.py %common_v8_test_options% %v8_test_options% --junitout ./v8-tap.xml'
-call python tools\run-tests.py %common_v8_test_options% %v8_test_options% --junitout ./v8-tap.xml
+echo running 'python tools\run-tests.py %common_v8_test_options% %v8_test_options% --slow-tests-cutoff 1000000 --json-test-results v8-tap.xml'
+call python tools\run-tests.py %common_v8_test_options% %v8_test_options% --slow-tests-cutoff 1000000 --json-test-results v8-tap.xml
+call python ..\..\tools\v8-json-to-junit.py < v8-tap.xml > v8-tap.json
 
 :test-v8-intl
 if not defined test_v8_intl goto test-v8-benchmarks
-echo running 'python tools\run-tests.py %common_v8_test_options% intl --junitout ./v8-intl-tap.xml'
-call python tools\run-tests.py %common_v8_test_options% intl --junitout ./v8-intl-tap.xml
+echo running 'python tools\run-tests.py %common_v8_test_options% intl --slow-tests-cutoff 1000000 --json-test-results v8-intl-tap.xml'
+call python tools\run-tests.py %common_v8_test_options% intl --slow-tests-cutoff 1000000 --json-test-results ./v8-intl-tap.xml
+call python ..\..\tools\v8-json-to-junit.py < v8-intl-tap.xml > v8-intl-tap.json
 
 :test-v8-benchmarks
 if not defined test_v8_benchmarks goto test-v8-exit
-echo running 'python tools\run-tests.py %common_v8_test_options% benchmarks --junitout ./v8-benchmarks-tap.xml'
-call python tools\run-tests.py %common_v8_test_options% benchmarks --junitout ./v8-benchmarks-tap.xml
+echo running 'python tools\run-tests.py %common_v8_test_options% benchmarks --slow-tests-cutoff 1000000 --json-test-results v8-benchmarks-tap.xml'
+call python tools\run-tests.py %common_v8_test_options% benchmarks --slow-tests-cutoff 1000000 --json-test-results ./v8-benchmarks-tap.xml
+call python ..\..\tools\v8-json-to-junit.py < v8-benchmarks-tap.xml > v8-benchmarks-tap.json
 goto test-v8-exit
 
 :test-v8-exit

This introduces some code to convert from V8's test JSON output to JUnit
XML. We need this because V8's latest refactor of their test runner has
made it difficult to float our JUnit reporter patch on top (see the
referenced issue).

I also think that there needs to be the same changes to vcbuild.bat, but
I don't know how to do test those yet. I can create a Windows VM and
test it if we decide to go with this approach.

Refs: nodejs/node-v8#236
@bnoordhuis
Copy link
Member

@kvakil I took the liberty of applying the patch and rebasing your commit on top of main. Let's see how it fares.

@targos
Copy link
Member

targos commented Sep 20, 2022

@nodejs/build-infra there seems to be an issue with test-nearform_intel-ubuntu1604-x64-1:

11:15:39 [0:00:42] fatal: unable to access 'https://chromium.googlesource.com/chromium/src/third_party/jinja2.git/': Could not resolve host: chromium.googlesource.com

@richardlau
Copy link
Member

@nodejs/build-infra there seems to be an issue with test-nearform_intel-ubuntu1604-x64-1:

11:15:39 [0:00:42] fatal: unable to access 'https://chromium.googlesource.com/chromium/src/third_party/jinja2.git/': Could not resolve host: chromium.googlesource.com

I'm not entirely sure what's wrong with it. I rebooted it and the "Could not resolve host" error hasn't reappeared but we are getting GnuTLS errors now

13:04:04 [0:05:50] Receiving objects:   0% (1028/124226), 771.77 KiB | 3.00 KiB/s   
13:04:04 [0:07:50] error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
13:04:04 [0:07:50] fatal: The remote end hung up unexpectedly
13:04:04 [0:07:50] fatal: early EOF
13:04:04 [0:07:50] fatal: index-pack failed

Although https://ci.nodejs.org/job/node-test-commit-v8-linux/4868/ (rerun of the CI for this PR) passed so the issue isn't consistent 🤷 .

@targos
Copy link
Member

targos commented Sep 20, 2022

So, node-test-commit-v8-linux isn't broken. Now I don't know how to try it with ENABLE_CONVERT_V8_JSON_TO_XML.

@targos
Copy link
Member

targos commented Sep 25, 2022

@richardlau do you have a suggestion about what we should do now?

@richardlau
Copy link
Member

@richardlau do you have a suggestion about what we should do now?

I've made some changes to the job:
Added

TAP_OUTPUT_OPTION="ENABLE_CONVERT_V8_JSON_TO_XML=True"
if (./deps/v8/tools/run-tests.py --help | grep -q junittout); then
  TAP_OUTPUT_OPTION="ENABLE_V8_TAP=True"
fi

and then replaced ENABLE_V8_TAP=True with "$TAP_OUTPUT_OPTION" in the variables passed to make.

@richardlau
Copy link
Member

@richardlau do you have a suggestion about what we should do now?

I've made some changes to the job: Added

TAP_OUTPUT_OPTION="ENABLE_CONVERT_V8_JSON_TO_XML=True"
if (./deps/v8/tools/run-tests.py --help | grep -q junittout); then
  TAP_OUTPUT_OPTION="ENABLE_V8_TAP=True"
fi

and then replaced ENABLE_V8_TAP=True with "$TAP_OUTPUT_OPTION" in the variables passed to make.

I typoed junitout 😅 . Also had to update to use the same version of Python that configure picked up.

@targos
Copy link
Member

targos commented Sep 29, 2022

Is https://ci.nodejs.org/job/node-test-commit-v8-linux/4882/ the new run?
It failed with 18:19:57 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

@richardlau
Copy link
Member

Is https://ci.nodejs.org/job/node-test-commit-v8-linux/4882/ the new run? It failed with 18:19:57 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

That run was the one that revealed I needed to amend the job to use the same version of Python that the configure script picked up.
https://ci.nodejs.org/job/node-test-commit-v8-linux/4884/ is the change with v14.x
https://ci.nodejs.org/job/node-test-commit-v8-linux/4885/ with v16.x
https://ci.nodejs.org/job/node-test-commit-v8-linux/4886/ is main

All of the above are without this PR but with the job changes. Since the job changes detect if the V8 test runner supports --junitout it will only use the new converter in this PR with the updated V8 that doesn't carry our junit reporter patch (e.g. in #44741).

I'm not sure what's up with the benchmark machine -- it looks like issues fetching V8 dependencies stretching back over a week (i.e. before any of these changes).

@targos
Copy link
Member

targos commented Sep 29, 2022

@targos
Copy link
Member

targos commented Sep 29, 2022

I'm not sure what's up with the benchmark machine -- it looks like issues fetching V8 dependencies stretching back over a week (i.e. before any of these changes).

Can we clear the workspace and try again? I don't remember how to do that in Jenkins.

@richardlau
Copy link
Member

I'm not sure what's up with the benchmark machine -- it looks like issues fetching V8 dependencies stretching back over a week (i.e. before any of these changes).

Can we clear the workspace and try again? I don't remember how to do that in Jenkins.

You go to the Workspace link for the job (note not the numbered runs) and then "wipe out workspace" from there, e.g.

I've wiped out the workspace for this job and started a new run off main: https://ci.nodejs.org/job/node-test-commit-v8-linux/4888

@richardlau
Copy link
Member

I've wiped out the workspace for this job and started a new run off main: https://ci.nodejs.org/job/node-test-commit-v8-linux/4888

Nope, doesn't appear to have fixed it 😞 .

18:47:07 [0:04:58] fatal: unable to access 'https://chromium.googlesource.com/chromium/tools/depot_tools.git/': Could not resolve host: chromium.googlesource.com

which is really odd because right at the beginning of the job we had a successful git clone of https://chromium.googlesource.com/chromium/tools/depot_tools.git

(we should probably spin this discussion out to a separate issue as it's not really anything to do with this PR.)

@targos
Copy link
Member

targos commented Sep 30, 2022

@targos
Copy link
Member

targos commented Sep 30, 2022

we should probably spin this discussion out to a separate issue as it's not really anything to do with this PR

I agree, this is an infra issue and shouldn't block this PR.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47029/

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 691f8d1 into nodejs:main Oct 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 691f8d1

panva pushed a commit to panva/node that referenced this pull request Oct 4, 2022
This introduces some code to convert from V8's test JSON output to JUnit
XML. We need this because V8's latest refactor of their test runner has
made it difficult to float our JUnit reporter patch on top (see the
referenced issue).

I also think that there needs to be the same changes to vcbuild.bat, but
I don't know how to do test those yet. I can create a Windows VM and
test it if we decide to go with this approach.

Refs: nodejs/node-v8#236
PR-URL: nodejs#44049
Fixes: nodejs/node-v8#236
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Oct 11, 2022
This introduces some code to convert from V8's test JSON output to JUnit
XML. We need this because V8's latest refactor of their test runner has
made it difficult to float our JUnit reporter patch on top (see the
referenced issue).

I also think that there needs to be the same changes to vcbuild.bat, but
I don't know how to do test those yet. I can create a Windows VM and
test it if we decide to go with this approach.

Refs: nodejs/node-v8#236
PR-URL: #44049
Fixes: nodejs/node-v8#236
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lost JUnit support for V8 tests
7 participants