test: fix test-api-getreport.js #58
Conversation
process.argv0 was added in Node.js v6.4.0 and doesn't exist in Node.js v4.
|
CI run with Node.js v4: https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/52/ |
|
LGTM. There are a couple of new failures in the CI run above, but they look like gzip problems with the node builds, not test issues. We need to improve the format of that verbose test output from the CI, I think @gibfahn has some ideas. |
|
We also need to get in the habit of testing PR's on the CI against Node.js v4 to catch stuff like this. |
|
Yes, we need to run the CI against Node 4, Node 6 and head I guess |
test/test-api-getreport.js
Outdated
| common.validateContent(report_str, tap, {pid: process.pid, | ||
| commandline: [process.argv0, 'test/test-api-getreport.js'].join(' ') | ||
| }); | ||
| commandline: [process.argv[0], 'test/test-api-getreport.js'].join(' ') |
There was a problem hiding this comment.
I'm not sure how https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/52/MACHINE=win10/console passed given that the slash here is incorrect for Windows (test is failing locally for me).
There was a problem hiding this comment.
ah it's because the test script in package.json uses forward slashes so the test does match:
"test": "tap test/test*.js"
I was using backslashes on the command line when running the individual test locally.
There was a problem hiding this comment.
hmm so while everything passes when run with npm test, it looks like other ways of running the test fails:
- If the test is manually run on Windows with backslashes,
e.g.
node test\test-api-getreport.js
- If the test is run manually anywhere other than the node-report root directory as the test always expects the test to be
test/test-api-getreport.js,
e.g.
-bash-4.2$ cd test
-bash-4.2$ pwd
/home/users/riclau/sandbox/github/nodereport/test
-bash-4.2$ node test-api-getreport.js
TAP version 13
1..17
ok 1 - Checking report contains Node Report section
ok 2 - Checking report contains JavaScript Stack Trace section
ok 3 - Checking report contains JavaScript Heap section
ok 4 - Checking report contains System Information section
ok 5 - Node Report header section contains expected process ID
ok 6 - Node Report header section contains expected Node.js version
not ok 7 - Checking report contains expected command line
---
found: >-
Event: JavaScript API, location: "GetReport"
Dump event time: 2017/02/14 18:35:19
Module load time: 2017/02/14 18:35:19
Process ID: 113131
Command line: node test-api-getreport.js
Node.js version: v4.7.3
(ares: 1.10.1-DEV, http_parser: 2.7.0, icu: 56.1, modules: 46, openssl:
1.0.2k,
uv: 1.9.1, v8: 4.5.103.43, zlib: 1.2.8)
node-report version: 2.0.0 (built against Node.js v4.7.3)
OS version: Linux 3.10.0-327.18.2.el7.x86_64 #1 SMP Fri Apr 8 05:09:53 EDT
2016
(glibc: 2.17)
Machine: drx-hemera x86_64
pattern: >-
/Command line: \/dev\/shm\/usenode.riclau\/node-v4.7.3-linux-x64\/bin\/node
test\/test-api-getreport.js/
at:
line: 77
column: 9
file: common.js
function: validateContent
stack: |
Object.validateContent (common.js:77:9)
Object.<anonymous> (test-api-getreport.js:9:8)
source: |
t.match(nodeReportSection,
...
Also note that Node.js seems to fully qualify process.argv[0], so process.argv0 would be the correct thing to use if it is available (but would not fix the test looking for test/test-api-getreport.js).
- If run with extra command line arguments,
e.g.
-bash-4.2$ node --max-old-space-size=1024 test/test-api-getreport.js
TAP version 13
1..17
ok 1 - Checking report contains Node Report section
ok 2 - Checking report contains JavaScript Stack Trace section
ok 3 - Checking report contains JavaScript Heap section
ok 4 - Checking report contains System Information section
ok 5 - Node Report header section contains expected process ID
ok 6 - Node Report header section contains expected Node.js version
not ok 7 - Checking report contains expected command line
---
found: >-
Event: JavaScript API, location: "GetReport"
Dump event time: 2017/02/14 18:43:24
Module load time: 2017/02/14 18:43:24
Process ID: 113385
Command line: node --max-old-space-size=1024 test/test-api-getreport.js
Node.js version: v4.7.3
(ares: 1.10.1-DEV, http_parser: 2.7.0, icu: 56.1, modules: 46, openssl:
1.0.2k,
uv: 1.9.1, v8: 4.5.103.43, zlib: 1.2.8)
node-report version: 2.0.0 (built against Node.js v4.7.3)
OS version: Linux 3.10.0-327.18.2.el7.x86_64 #1 SMP Fri Apr 8 05:09:53 EDT
2016
(glibc: 2.17)
Machine: drx-hemera x86_64
pattern: >-
/Command line: \/dev\/shm\/usenode.riclau\/node-v4.7.3-linux-x64\/bin\/node
\/home\/users\/riclau\/sandbox\/github\/nodereport\/test\/test-api-getreport.js/
at:
line: 77
column: 9
file: test/common.js
function: validateContent
stack: |
Object.validateContent (test/common.js:77:9)
Object.<anonymous> (test/test-api-getreport.js:9:8)
source: |
t.match(nodeReportSection,
...
The other tests that check the expected command line are creating child processes to get the report from, so know exactly what command line is invoked. This test doesn't have that control, so is less robust if run in ways other than npm test. Are we concerned about these?
There was a problem hiding this comment.
Couldn't you just check that test/test-api-getreport.js exists, and try test\test-api-getreport.js otherwise (and fail if neither exist)?
process.argv[0] seems like the way to go for your other issue.
There was a problem hiding this comment.
That would address 1. but not 2.
There was a problem hiding this comment.
Could you use process.argv[1] to get the path to the test?
There was a problem hiding this comment.
Unfortunately not, Node.js resolves it like it does argv[0]:
-bash-4.2$ cat test.js
console.log(process.argv)
-bash-4.2$ node test.js
[ '/dev/shm/usenode.riclau/node-v4.7.3-linux-x64/bin/node',
'/home/users/riclau/sandbox/scratch/test/test.js' ]
-bash-4.2$
There was a problem hiding this comment.
I think the best option would be to assume the test path was the direct relative path to the test. You should be able to use process.title instead of process.argv0 (I couldn't get this to not work, but I didn't try windows, YMMV).
diff --git a/test/test-api-getreport.js b/test/test-api-getreport.js
index a36192e..5fdfab8 100644
--- a/test/test-api-getreport.js
+++ b/test/test-api-getreport.js
@@ -5,7 +5,9 @@
const common = require('./common.js');
const tap = require('tap');
const nodereport = require('../');
+
+const testPath = process.argv[1].substr(process.cwd().length + 1);
var report_str = nodereport.getReport();
common.validateContent(report_str, tap, {pid: process.pid,
- commandline: [process.argv0, 'test/test-api-getreport.js'].join(' ')
-});
\ No newline at end of file
+ commandline: [process.title, testPath].join(' ')
+});
Obviously that wouldn't handle things like node $PWD/test-api-getreport.js, or weird things like node test/../test/test-api-getreport.js, but it should hopefully handle windows and manually running the tests (i.e. all common situations).
Spawn child process so the reported command line can be reliably tested. Check that no report files or stderr messages are written.
|
I've rewritten the test case so that it spawns a child process -- This seems the safest way to be able to reliably test the command line in the report. It also allows the test to assert that no messages are written to stderr and that no report files are written. Node.js v4 CI: https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/85/ cc @rnchamberlain since I changed the test since your LGTM. |
|
You man need to start the v4 run again its possible you got unlucky and the nightly builds were underway during your test. If the failure persists let me know. |
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM provided the CI's are green.
|
Looks good, planning to land this shortly |
process.argv0 was added in Node.js v6.4.0 and doesn't exist in Node.js v4. PR-URL: #58 Reviewed-By: Richard Chamberlain <richard_chamberlain@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
Landed as f170edf |
4 participants
I missed this when reviewing #48 😞. The new test fails on Node.js v4: https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/51/
e.g. https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/51/MACHINE=ubuntu1404-64/console
The test is using
process.argv0, which doesn't exist in Node.js v4.I also fixed up the comment in the testcase in light of the rename.