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

Broken V8 CI #100

Closed
targos opened this issue Feb 8, 2019 · 15 comments
Closed

Broken V8 CI #100

targos opened this issue Feb 8, 2019 · 15 comments

Comments

@targos
Copy link
Member

targos commented Feb 8, 2019

https://ci.nodejs.org/job/node-test-commit-v8-linux/2081/nodes=benchmark,v8test=v8test/console

Error:

12:22:20 deps/v8/tools/run-tests.py --gn --arch=x64 \
12:22:20         --mode=release --progress=dots --timeout=120 \
12:22:20 				mjsunit cctest debugger inspector message preparser \
12:22:20 	      --junitout /home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark/v8test/v8test/v8-tap.xml
12:22:20 Usage: run-tests.py [options] [tests]
12:22:20 
12:22:20 run-tests.py: error: no such option: --junitout
@targos
Copy link
Member Author

targos commented Feb 8, 2019

Support for junit output was removed in v8/v8@bd019bd

@refack
Copy link

refack commented Feb 8, 2019

Who do we talk to to get this back. JUnit is the lingua franca of Jenkins...

@nodejs/v8-update

@hashseed
Copy link
Member

hashseed commented Feb 8, 2019

I wasn't aware this was used. Is there no alternative? It's hard to maintain features we don't test upstream.

@refack
Copy link

refack commented Feb 8, 2019

Ironically I wanted to port it to node-core's test.py, since ATM we output TAP and transform it to JUnit...

From what I see in that file you also dropped TAP so the only machine readable output format is your proprietary JSON?
https://github.com/v8/v8/blob/master/tools/testrunner/testproc/progress.py#L249

An alternative could be to write a transformer for that, but IMHO restoring those dropped 95 lines would be the lowest hanging fruit. (For which you get CI test coverage from us ;)

@refack
Copy link

refack commented Mar 17, 2019

No expected resolution?

@hashseed
Copy link
Member

@tmrts we should consider readding junit support.

@tmrts
Copy link

tmrts commented Mar 18, 2019

@hashseed agreed, I'll readd it this week.

Thanks for the details @targos @refack!

@refack
Copy link

refack commented Mar 18, 2019

@tmrts, thanks for your consideration!

@refack
Copy link

refack commented May 28, 2019

ping? any news of this?

@refack
Copy link

refack commented May 28, 2019

BTW you can rename this xUnit as the format has been adopted by multiple tools for several runtimes - https://en.wikipedia.org/wiki/XUnit#Test_result_formatter

@mmarchini
Copy link

Is there anything left to be done here?

@targos
Copy link
Member Author

targos commented Nov 3, 2020

Either someone tries to upstream the revert: c58cd35#diff-f93464a48a2b9281080b555fac1081b1852154552800cbc6a2440ad1e3629033
Or we accept to float it forever and close this issue.

@mhdawson
Copy link
Member

mhdawson commented Nov 9, 2020

As I understand it the only impact is that we don't get the test results in a format that we'd like. We still run/get results and have coped for 1 1/2 years. I figure we just close. @targos make sense to you?

@targos targos closed this as completed Feb 20, 2021
@cclauss
Copy link

cclauss commented Feb 20, 2021

% flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

./v8/tools/dev/update-compile-commands.py:55:25: F821 undefined name 'Error'
    if code != 0: raise Error("gn gen failed")
                        ^
./v8/tools/clusterfuzz/js_fuzzer/tools/minimize.py:26:1: F631 assertion is always true, perhaps remove parentheses?
assert(len(sys.argv) > 1, 'Need to specify minimizer path.')
^
1     F631 assertion is always true, perhaps remove parentheses?
1     F821 undefined name 'Error'
2

Fixes needed:

    if code != 0: raise Exception("gn gen failed")
...and...
assert len(sys.argv) > 1, 'Need to specify minimizer path.'

@targos
Copy link
Member Author

targos commented Feb 20, 2021

@cclauss That's unrelated. This issue was about a patch that we have to float on top of V8 to keep support for junit test output.

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

No branches or pull requests

7 participants