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

lint job doesn't show tap result #720

Closed
refack opened this issue May 13, 2017 · 12 comments
Closed

lint job doesn't show tap result #720

refack opened this issue May 13, 2017 · 12 comments

Comments

@refack
Copy link
Contributor

refack commented May 13, 2017

Maybe I'm doing something wrong, but I can't figure out how to get the lint errors...

e.g. https://ci.nodejs.org/job/node-test-linter/9042/console

+ gmake lint-ci
Running JS linter...
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
	benchmark doc lib test tools
gmake: *** [Makefile:870: jslint-ci] Error 1
Build step 'Execute shell' marked build as failure
Run condition [Always] enabling perform for step [[Execute a set of scripts]]
[PostBuildScript] - Execution post build scripts.
[PostBuildScript] Build is not success : do not execute script
Run condition [Always] enabling perform for step [[Execute a set of scripts]]
[PostBuildScript] - Execution post build scripts.
[node-test-linter] $ /bin/sh -xe /tmp/hudson6596229026728241521.sh
+ set +x
Notifying upstream projects of job completion
Finished: FAILURE

Sorry to fast fire issues today.

@gibfahn
Copy link
Member

gibfahn commented May 13, 2017

You're right, we're passing the output to a TAP file, but not actually running the TAP parser on it. I tried enabling it, but the TAP plugin fails to parse it as it isn't valid TAP.

Looking for TAP results report in workspace using pattern: test-eslint.tap
Saving reports...
org.tap4j.parser.ParserException: Error parsing TAP Stream: Duplicated TAP Header found.
	at org.tap4j.parser.Tap13Parser.parseTapStream(Tap13Parser.java:230)
Caused by: org.tap4j.parser.ParserException: Duplicated TAP Header found.
	at org.tap4j.parser.Tap13Parser.parseLine(Tap13Parser.java:320)
	at org.tap4j.parser.Tap13Parser.parseTapStream(Tap13Parser.java:224)
	... 15 more
TAP parse errors found in the build. Marking build as UNSTABLE

The problem with the generated TAP output is that it's actually multiple TAP files concatenated together, so there are multiple TAP headers.

TAP version 13
1..1
ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/vm/run-in-context.js

TAP version 13
1..1
ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/vm/run-in-this-context.js

TAP version 13
1..2
ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/util/normalize-encoding.js
ok 2 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/v8/get-stats.js

In the short term you can view the file directly (assuming you have the permissions) by going to the workspace.

@gibfahn
Copy link
Member

gibfahn commented May 13, 2017

Looks like we can definitely blame @jbergstroem for this 😜

@mscdex nodejs/node#5638 (comment)

@jbergstroem Does the tap output have to all be in one single group of results (e.g. only one TAP header in the output file) or does it not matter?


@jbergstroem nodejs/node#5638 (comment)

@mscdex I recall globbing files ("collect *.tap") working just fine.

@gibfahn
Copy link
Member

gibfahn commented May 13, 2017

So basically we need to update tools/jslint.js to remove all but the first instance of TAP version 13, I think that should work.

@refack
Copy link
Contributor Author

refack commented May 13, 2017

I'm on it.

P.S. the workspace will only show that last run, right?

@gibfahn
Copy link
Member

gibfahn commented May 13, 2017

P.S. the workspace will only show that last run, right?

Yes, it's overwritten every time.

@gibfahn
Copy link
Member

gibfahn commented May 14, 2017

The plan cannot appear in the middle of the output, nor can it appear more than once.

I lied, the plan can't be repeated, I tested it and it still breaks the TAP parsing plugin. The easiest solution is to use Subtests that Jenkins can understand (see the plugin docs):

I've confirmed that this works in a test job:

TAP version 13
  1..9
  ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/legacy-vs-whatwg-url-searchparams-serialize.js
  ok 2 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/legacy-vs-whatwg-url-serialize.js
  ok 3 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/url-format.js
  ok 4 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/url-resolve.js
  ok 5 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/url-searchparams-iteration.js
  ok 6 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/url-searchparams-read.js
  ok 7 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/url-searchparams-sort.js
  ok 8 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/usvstring.js
  ok 9 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/whatwg-url-idna.js
ok 1 Second set  
  1..4
  ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/url/whatwg-url-properties.js
  ok 2 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/util/format.js
  ok 3 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/util/inspect-proxy.js
  ok 4 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/util/inspect.js
ok 2 Third set  
  1..1
  ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/vm/run-in-context.js
ok 3 Fourth set
  1..1
  ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/vm/run-in-this-context.js
ok 4 Fifth set
  1..2
  ok 1 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/util/normalize-encoding.js
  ok 2 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/v8/get-stats.js
ok 5 sixth set
1..5

@refack
Copy link
Contributor Author

refack commented May 14, 2017

I started writing the PR... ✍️

@gibfahn
Copy link
Member

gibfahn commented Oct 7, 2017

I changed

gmake lint-ci

to

gmake lint-ci || { cat test-eslint.tap && exit 1; }

, so if the test fails you'll get the test output catted to the console.

It's not the best solution, but I think it solves the problem in the immediate term.

@gibfahn
Copy link
Member

gibfahn commented Oct 8, 2017

New solution (to hopefully avoid #906) and make it easier to see what's going on:

#!/usr/bin/env bash -ex
# If lint-ci fails, print all the interesting lines to the console.
# FreeBSD sed can't handle \s, so use gsed if it exists.
which gsed &>/dev/null && SED=gsed || SED=sed
gmake lint-ci || { 
  cat test-eslint.tap | grep -v '^ok\|^TAP version 13\|^1\.\.' | $SED '/^\s*$/d' && 
  exit 1; }

Filters out the passing tests, the TAP version 13 headers, and the 1..10 lines.

@refack
Copy link
Contributor Author

refack commented Oct 8, 2017

grep -v -P "^(ok|1\.\.|TAP|$)" test-eslint.tap

@gibfahn
Copy link
Member

gibfahn commented Oct 8, 2017

Updated comment above, the script got a bit more complicated because:

  • /bin/bash doesn't exist
  • POSIX sed doesn't support \s for whitespace.
  • The linter runs on two boxes, one is test-rackspace-freebsd10-x64-1 which has gsed, the other istest-joyent-freebsd10-x64-2 which doesn't.

So at the moment on the joyent box it doesn't trim whitespace, and on the other rackspace box it does. If we could:

  • Work out the POSIX equivalent of sed '/^\s*$/d' is (and test it on the joyent box)
  • Install gsed on the joyent box

That would improve the situation.

At the moment the freebsd box trims blank lines, but the joyent one doesn't. It's better than before, but not ideal.

@gibfahn gibfahn changed the title lint job doesn't show tap resault lint job doesn't show tap result Oct 8, 2017
@gibfahn
Copy link
Member

gibfahn commented Oct 22, 2017

This is done for now, better solutions always appreciated.

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

2 participants