Convert testcases to use tap #15
Conversation
| 'System Information' | ||
| ]; | ||
|
|
||
| exports.locate = (pid) => { |
There was a problem hiding this comment.
its helpful to have a comment showing the file format you are looking for with this kind of code
| return files[i]; | ||
| } | ||
| } | ||
| return; |
There was a problem hiding this comment.
This was how it was in the original autorun.js. I'll get rid of it.
| }; | ||
|
|
||
| exports.validate = (t, report) => { | ||
| const reportStream = fs.createReadStream(report); |
There was a problem hiding this comment.
this is what fs.readFile() or fs.readFileSync() do
| const tap = require('tap'); | ||
| const common = require('./common.js'); | ||
| const fs = require('fs'); | ||
| const spawn = require('child_process').spawn; |
There was a problem hiding this comment.
its typical to lexically sort require lines by the name of the var
There was a problem hiding this comment.
Will do so for all of the tests.
| const spawn = require('child_process').spawn; | ||
|
|
||
| var child = spawn(process.execPath, [__filename, 'child']); | ||
| child.on('exit', function(code, signal) { |
| tap.ok(report, 'found report ' + report); | ||
| if (report) { | ||
| common.validate(tap, report); | ||
| // delete the report when done |
There was a problem hiding this comment.
why? just makes debugging harder, its ok for tests to leave ephemra on disk, CI cleans it up afterwards
| var child = spawn(process.execPath, [__filename, 'child']); | ||
| child.on('exit', function(code, signal) { | ||
| tap.plan(3); | ||
| tap.notEqual(code, 0, 'Process should not exit cleanly'); |
There was a problem hiding this comment.
I would be more assertive, child should exit with status 1, anything else means nodereport damaged the normal return status for uncaught errors
There was a problem hiding this comment.
This was based on the original autorun.js but it's good that we can be more assertive so I'll change.
There was a problem hiding this comment.
Well that didn't work (tried Node v4.6.1, v6.9.1 on x64 Linux):
-bash-4.2$ ./node_modules/.bin/tap --reporter=tap test/test-exception.js
TAP version 13
# Subtest: test/test-exception.js
1..3
not ok 1 - Process should exit with status 1
---
found: null
wanted: 1
compare: ===
at:
line: 26
column: 9
file: test/test-exception.js
type: ChildProcess
stack: |
ChildProcess.<anonymous> (test/test-exception.js:26:9)
source: |
tap.equal(code, 1, 'Process should exit with status 1');
...
ok 2 - found report NodeReport.20161102.160920.8809.001.txt
# Subtest: validating NodeReport.20161102.160920.8809.001.txt
1..4
ok 1 - Report contains NodeReport section
ok 2 - Report contains JavaScript Stack Trace section
ok 3 - Report contains JavaScript Heap section
ok 4 - Report contains System Information section
ok 3 - validating NodeReport.20161102.160920.8809.001.txt # time=15.683ms
# failed 1 of 3 tests
# time=763.704ms
not ok 1 - test/test-exception.js # time=971.072ms
---
timeout: 30000
results:
ok: false
plan:
start: 1
end: 3
pass: 2
fail: 1
count: 3
arguments:
- test/test-exception.js
file: test/test-exception.js
exitCode: 1
command: /dev/shm/usenode.riclau/node-v4.6.1-linux-x64/bin/node
...
1..1
# failed 1 of 1 tests
# time=1005.135ms
-bash-4.2$
and
-bash-4.2$ node test/test-exception.js child ; echo $?
Writing Node.js report to file: NodeReport.20161102.161145.9112.001.txt
Node.js report completed
Uncaught #<UserException>
FROM
myException (/home/users/riclau/sandbox/github/nodereport/test/test-exception.js:8:5)
Object.<anonymous> (/home/users/riclau/sandbox/github/nodereport/test/test-exception.js:16:3)
Module._compile (module.js:409:26)
Object.Module._extensions..js (module.js:416:10)
Module.load (module.js:343:32)
Function.Module._load (module.js:300:12)
Function.Module.runMain (module.js:441:10)
startup (node.js:139:18)
node.js:974:3
Illegal instruction (core dumped)
132
-bash-4.2$
Guess this is a bug? cc @rnchamberlain
There was a problem hiding this comment.
Maybe more of a "how Sam thinks it should work" :-). I'd do tap.notEqual(signal || code, 0, ... <-- note carefully that signal is before code, because 0 is a valid code and is false. Might have to change this back for now.
There was a problem hiding this comment.
I mean, change the test back to assert the original behaviour, maybe with a comment that perhaps the behaviour of !== 0 is not correct.
There was a problem hiding this comment.
Re "Guess this is a bug" - assuming this is a reference toIllegal instruction (core dumped)
That's the v8 behaviour when --abort-on-uncaught-exception is set, and an exception is thrown and not caught. You can see it without nodereport interference by running:
node --abort-on-uncaught-exception unknown.js
v8::base::OS::Abort() intentionally aborts and triggers a core dump using an illegal instruction. It's what they call a 'hard' abort. Not sure why, in the JVM we use raise(SIGABRT), but maybe there are some situations where that gets intercepted.
However, see also discussion in #6 We need to change the default nodereport behaviour, currently it adds the --abort-on-uncaught-exception flag under the covers, and the user has to switch that off via the nodereport.setCoreDump() APi if they want normal Node.js behaviour.
There was a problem hiding this comment.
Okay, I'm going to make the test assert no exit code (null) and SIGILL signal as that is the current expected behaviour. We should then change it if we change the default nodereport behaviour.
There was a problem hiding this comment.
Of course the behaviour is different on Windows.
C:\work\node\github\nodereport>node_modules\.bin\tap.cmd test\test-exception.js
test\test-exception.js ................................ 5/7
not ok Process should not exit cleanly
+++ found
--- wanted
-[null]
+3221225477
compare: ===
at:
line: 27
column: 9
file: test/test-exception.js
type: ChildProcess
stack: |
ChildProcess.<anonymous> (test/test-exception.js:27:9)
source: |
tap.equal(code, null, 'Process should not exit cleanly');
not ok Process should exit with signal SIGILL
+++ found
--- wanted
-"SIGILL"
+[null]
compare: ===
at:
line: 28
column: 9
file: test/test-exception.js
type: ChildProcess
stack: |
ChildProcess.<anonymous> (test/test-exception.js:28:9)
source: >
tap.equal(signal, expectedSignal, 'Process should exit with signal ' +
expectedSignal);
total ................................................. 5/7
5 passing (1s)
2 failing
C:\work\node\github\nodereport>
| const fs = require('fs'); | ||
| const spawn = require('child_process').spawn; | ||
|
|
||
| var child = spawn(process.execPath, ['--max-old-space-size=20', __filename, 'child']); |
There was a problem hiding this comment.
I see, you limit the old space to speed up the test, ignore last comment.
| @@ -0,0 +1,36 @@ | |||
| 'use strict'; | |||
|
|
|||
| // Testcase to produce NodeReport on uncaught exception | |||
There was a problem hiding this comment.
you could do a variant of this that emits 'error' on an EventEmitter, too
There was a problem hiding this comment.
I think I would prefer for this PR to concentrate on converting the existing tests to use tap. We can then add more variants and new tests once this is merged.
| const spawn = require('child_process').spawn; | ||
|
|
||
| if (process.platform === 'win32') { | ||
| tap.plan(0, 'Signals not supported on Windows'); |
There was a problem hiding this comment.
some test parsers fail when there are no asserts, I would change this to a tap.ok(false, {skip: 'unsupported on windows'), ... or something of the sort
|
|
||
| if (process.platform === 'win32') { | ||
| tap.plan(0, 'Signals not supported on Windows'); | ||
| process.exit(0); |
There was a problem hiding this comment.
change to return, exit isn't safe, it can occur before file i/o has completed
|
|
||
| function myLoop() { | ||
| var list = []; | ||
| for (var i=0; i<10000000000; i++) { |
There was a problem hiding this comment.
there are some indentations problems here
what are you trying to do? are you attempting to show that signals interrupt a js busy-loop?
I think an interesting test would be having the child also listen on SIGUSR1, and show that both nodereport and the child get the signal (I assume they should, but nodereport may have stolen the signal from uv or node, good to show it did not)
There was a problem hiding this comment.
Not entirely sure what this is trying to do -- @rnchamberlain ?
There was a problem hiding this comment.
Yes, this puts the test application into a busy loop, and we are testing that we can get a NodeReport using a signal in that scenario.
The other key scenario for a signal test is an application in idle/waiting, it would be good to have a test for that at some point.
| setTimeout(() => { | ||
| child.kill('SIGTERM'); | ||
| }, 1000); | ||
| }, 1000); |
There was a problem hiding this comment.
timeouts are touchy in tests. on arm, it may take more than a second to start the child, and then this will break
better to wait asynchronously for the events you depend on. I would fork the child, not spawn it, or spawn it with an ipc channel and have the child send a msg to the parent when it is ready
then I would have the parent loop, looking at the filesystem, waiting for common.locate() to return a file
this might run a lot faster in the normal case, and also pass on really slow systems
|
I think this is great, the tests are much easier to read and understand. |
| exports.locate = (pid) => { | ||
| // Default NodeReport filenames are of the form NodeReport.<date>.<time>.<pid>.<seq>.txt | ||
| var files = fs.readdirSync("."); | ||
| return files.find((file) => { |
There was a problem hiding this comment.
I've left this for now returning the first match, but we probably want to be more strict and know the number of reports we expect to see (most likely 1) and check accordingly.
|
Some nits addressed. I've left the exit code check for |
|
v8::base::OS::Abort() intentionally aborts and triggers a core dump using an illegal instruction. It's what they call a 'hard' abort. Not sure why, in the JVM we use raise(SIGABRT), but maybe there are some situations where that gets intercepted.
@bnoordhuis, do you know why v8 does this, rather than calling
`abort(3)` or raising `SIGABRT` itself?
|
|
Probably because chromium works the same way. If your question is why chromium does it - I don't know. :-) You can make it SIGABRT instead of SIGILL by passing |
|
Windows doesn't have signals. There is some partial emulation, but yes, on
windows it will exit with a status here. I think the very original
assertion that `code !== 0` is probably best.
|
richardlau
force-pushed
the
tap-tests
branch
3 times, most recently
from
3daa339
to
567f957
Compare
|
Okay,
I'd like to focus this PR on converting the existing tests to use the tap plugin and keep additional tests for other PR's. |
|
Fair enough. I'll re-review, want to squash it first? |
|
@sam-github squashed, thanks. |
| const plan = REPORT_SECTIONS.length + (pid ? 1 : 0); | ||
| t.plan(plan); | ||
| if (pid) { | ||
| t.match(reportContents, new RegExp('Process ID: ' + pid), 'Checking report contains expected process ID ' + pid); |
There was a problem hiding this comment.
Line to long. We need to add eslint to this repo, but in meantime, make it easy on whoever does that and limit to 80 columns.
There was a problem hiding this comment.
Okay, will go through all changed files and cap the line length to 80 columns.
| child.on('exit', (code) => { | ||
| tap.plan(3); | ||
| tap.equal(code, 0, 'Process exited cleanly'); | ||
| // Locate and validate the NodeReport |
There was a problem hiding this comment.
I wouldn't have this comment. Comment says
Locate and validate the NodeReport
code says
findReports ... validate(... report )...
so comment is just rewording the code. Just my thoughts.
There was a problem hiding this comment.
Agreed, comment is redundant and will remove.
richardlau
force-pushed
the
tap-tests
branch
2 times, most recently
from
7ff7f47
to
657ca79
Compare
|
Addressed latest suggestions (mainly capping line length at 80 columns).
|
| // Default filenames are of the form NodeReport.<date>.<time>.<pid>.<seq>.txt | ||
| const format = '^NodeReport\\.\\d+\\.\\d+\\.' + pid + '\\.\\d+\\.txt$'; | ||
| const filePattern = new RegExp(format); | ||
| const files = fs.readdirSync("."); |
There was a problem hiding this comment.
Core style is to prefer single quote strings whenever possible.
|
|
||
| // Testcase to produce NodeReport on uncaught exception | ||
| if (process.argv[2] === 'child') { | ||
| require('../').setEvents("exception"); |
There was a problem hiding this comment.
Ditto. I'll stop pointing it out.
| if (process.argv[2] === 'child') { | ||
| require('../').setEvents("fatalerror"); | ||
|
|
||
| var list = []; |
|
|
||
| function busyLoop() { | ||
| var list = []; | ||
| for (var i=0; i<10000000000; i++) { |
There was a problem hiding this comment.
Spaces around = and < and maybe write the large number as 1e10 for better legibility.
| } | ||
|
|
||
| process.send('child started'); | ||
| busyLoop(); |
There was a problem hiding this comment.
This should probably be process.send('child started', busyLoop). If for some reason the message can't be sent right away, it's going to get queued up but the queue will never be dispatched if you start busy looping immediately afterwards.
|
@richardlau many thanks for this re-write, looks great, and the tap output is really nice. Running on node v7.0.0, linux intel, the test-fatal-error.js one hangs, and the framework times out and reports after 30 secs i think, which is good. With #16 the failure goes away and all tests pass. |
|
Rebasing on current master. Temporarily copied I'll fix all of these in addition to those raised by @bnoordhuis. |
|
Fixed all the eslint problems and comments from @bnoordhuis PTAL. |
|
@richardlau I created https://www.npmjs.com/package/node-style to help with cases like this, where people want to use the node core style. I am happy to contribute a PR with it added as a lint script and pre-test script if you all would like? |
|
@geek, @sam-github said he would PR eslint rules but I don't see any reason why you shouldn't submit a PR if you're willing. I think there's a consensus that we should be linting, and it makes sense to follow Node.js' style as that's probably what the likely contributors to this project will be used to. 😄 |
|
@geek go for it, thanks for the offer |
|
ping @bnoordhuis |
|
LGTM One thing for debate sometime later. This coding style creates anonymous functions, which I think should be on the list of things to avoid if you want easy and useful post-mortem diagnostics. |
|
@sam-github and @richardlau PR into this PR: richardlau#1 |
|
There are a lot of PRs in play at once, I suggest rebasing this one and merging it ASAP, its all improvement, and been LGTMed. Then the other PRs like @geek's lint one can be rebased onto master, and have better chances of passing in tests. @rnchamberlain I don't think there are any problems with anonymous functions in test code, and I'm unconvinced they are the problem some people think they are. Whether I see a stack trace with the word validate in it, or not in it, is pretty much irrelevant to me. The name doesn't tell me that much, I'm going to have to look at the code to see why its broken, and I don't locate the code by function name, I locate it by file and line. I understand why node core is cleaning this up so that it all looks a bit prettier, but the idea that seeing some internal function name is going to help a lot is a bit overplayed, IMO. And doesn't matter at all in test code, where the readability of the test code is more important. Now, whether es6 function notation is more readable, not going to get into that debate.(EDIT: ok, I got into the debate, but I recognize I have lost it already :-). Its clearly the wave of the future, I'm coming to terms with it. And I'm learning to debug code that tried to bind an es6 function, but failed, so |
|
I think this PR is current and doesn't need rebasing (unless something else lands while I'm typing). Not sure if @bnoordhuis needs to approve since he requested changes previously (which I believe have been addressed now). |
|
I think we should give @bnoordhuis a few more days to review. I'll try to see if I can contact him. |
|
master has been updated, so I have rebased. |
|
|
||
| const child = spawn(process.execPath, [__filename, 'child']); | ||
| child.on('exit', (code, signal) => { | ||
| const expectedExitCode = process.platform === 'win32' ? 3221225477 : null; |
There was a problem hiding this comment.
Just a suggestion, but 0xC0000005 is arguably more recognizable than 3221225477.
richardlau
force-pushed
the
tap-tests
branch
from
8296c3c
to
5f28704
Compare
|
Rebased onto master again and changed |
PR-URL: #15 Reviewed-By: Richard Chamberlain <richard_chamberlain@uk.ibm.com> Sam Roberts <vieuxtech@gmail.com> Ben Noordhuis <info@bnoordhuis.nl>
|
Landed as 9af14e2 |
Convert existing testcases to use the tap module. This allows a variety of output formats, some of which are more friendly to CI systems like Jenkins.
Sample output