Skip to content
This repository was archived by the owner on Jun 18, 2021. It is now read-only.

test: fix test-api-getreport.js #58

Closed
richardlau wants to merge 2 commits into from
Closed

test: fix test-api-getreport.js #58

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bba89d
test: fix test-api-getreport.js
richardlau Feb 10, 2017
f2e35a2
test: Update test-api-getreport.js
richardlau Feb 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
test: fix test-api-getreport.js 
process.argv0 was added in Node.js v6.4.0 and doesn't exist in Node.js v4.
  • Loading branch information
@richardlau
richardlau committed Feb 10, 2017
commit 2bba89d1560c0ea139125fbb8bd3cd957c7ba74e
6 changes: 3 additions & 3 deletions test/test-api-getreport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
'use strict';

// Testcase for returning NodeReport as a string via API call
// Testcase for returning report as a string via API call

const common = require('./common.js');
const tap = require('tap');
const nodereport = require('../');
var report_str = nodereport.getReport();
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(' ')
Copy link
Copy Markdown
Member Author

@richardlau richardlau Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

@richardlau richardlau Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@richardlau richardlau Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so while everything passes when run with npm test, it looks like other ways of running the test fails:

  1. If the test is manually run on Windows with backslashes,
    e.g.
node test\test-api-getreport.js
  1. 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).

  1. 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?

Copy link
Copy Markdown
Member

@gibfahn gibfahn Feb 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@richardlau richardlau Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would address 1. but not 2.

Copy link
Copy Markdown
Member

@gibfahn gibfahn Feb 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use process.argv[1] to get the path to the test?

Copy link
Copy Markdown
Member Author

@richardlau richardlau Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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$

Copy link
Copy Markdown
Member

@gibfahn gibfahn Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

});