test: skip test-process-config if no config.gypi #16436

gibfahn · 2017-10-24T09:56:48Z

If you run the tests in a different machine to the one you built on,
this test will fail. Avoid this by skipping if the file doesn't exist.
We shouldn't need to check that the file exists in this test, as the
build won't pass without a config.gypi anyway.

Also adds console.logs, so you can see what the actual difference
between the objects was, as assert.deepStrictEqual() only shows you
the first three lines.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

gibfahn · 2017-10-24T09:57:14Z

FWIW the motivation for this is running the tests in a docker container.

richardlau · 2017-10-24T12:01:41Z

test/parallel/test-process-config.js

+// If the assert fails, it only shows 3 lines. We need all the output to
+// compare.
+console.log('config:', config);
+console.log('process.config:', process.config);


Are these necessary?

I believe as the comment states, in case the assertion in L60 fails the output it will show will be truncated. This logging is so that we'll have the raw input.
This could be minimized by wrapping the assert in a try ... catch and log in the catch before rethrowing

See the comment. If the test passes nothing gets logged. If the test fails the assert just logs the first three lines, which is completely useless.

When this test fails it's usually because something is subtly different in your config, and you need to diff the two files to find out what it is.

This could be minimized by wrapping the assert in a try ... catch and log in the catch before rethrowing

Good idea.

If the test fails the assert just logs the first three lines, which is completely useless.

Sounds like a problem with assert.deepStrictEqual, but that should be addressed elsewhere.

Added a commit to wrap in try/catch.

I think the better fix is to use common.log(), but we haven't implemented that yet. I'll raise a PR for it later.

refack · 2017-10-24T15:08:22Z

test/parallel/test-process-config.js

+// If the assert fails, it only shows 3 lines. We need all the output to
+// compare.
+console.log('config:', config);
+console.log('process.config:', process.config);


I believe as the comment states, in case the assertion in L60 fails the output it will show will be truncated. This logging is so that we'll have the raw input.
This could be minimized by wrapping the assert in a try ... catch and log in the catch before rethrowing

refack

🏆

gibfahn · 2017-10-25T15:45:47Z

CI: https://ci.nodejs.org/job/node-test-commit/13448/

refack · 2017-10-27T22:49:30Z

Follow up (just found this up):

node/tools/test.py

Line 941 in 22882d4

with open('config.gypi', 'r') as f:

If you run the tests in a different machine to the one you built on, this test will fail. Avoid this by skipping if the file doesn't exist. We shouldn't need to check that the file exists in this test, as the build won't pass without a config.gypi anyway. Also adds console.logs, so you can see what the actual difference between the objects was, as `assert.deepStrictEqual()` only shows you the first three lines. PR-URL: #16436 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

If you run the tests in a different machine to the one you built on, this test will fail. Avoid this by skipping if the file doesn't exist. We shouldn't need to check that the file exists in this test, as the build won't pass without a config.gypi anyway. Also adds console.logs, so you can see what the actual difference between the objects was, as `assert.deepStrictEqual()` only shows you the first three lines. PR-URL: nodejs/node#16436 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

If you run the tests in a different machine to the one you built on, this test will fail. Avoid this by skipping if the file doesn't exist. We shouldn't need to check that the file exists in this test, as the build won't pass without a config.gypi anyway. Also adds console.logs, so you can see what the actual difference between the objects was, as `assert.deepStrictEqual()` only shows you the first three lines. PR-URL: #16436 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: nodejs#16621 Refs: nodejs#16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

If you run the tests in a different machine to the one you built on, this test will fail. Avoid this by skipping if the file doesn't exist. We shouldn't need to check that the file exists in this test, as the build won't pass without a config.gypi anyway. Also adds console.logs, so you can see what the actual difference between the objects was, as `assert.deepStrictEqual()` only shows you the first three lines. PR-URL: #16436 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

If you run the tests in a different machine to the one you built on, this test will fail. Avoid this by skipping if the file doesn't exist. We shouldn't need to check that the file exists in this test, as the build won't pass without a config.gypi anyway. Also adds console.logs, so you can see what the actual difference between the objects was, as `assert.deepStrictEqual()` only shows you the first three lines. PR-URL: nodejs/node#16436 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: #16621 Refs: #16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

gibfahn added the test Issues and PRs related to the tests. label Oct 24, 2017

gireeshpunathil approved these changes Oct 24, 2017

View reviewed changes

richardlau reviewed Oct 24, 2017

View reviewed changes

jasnell approved these changes Oct 24, 2017

View reviewed changes

refack approved these changes Oct 24, 2017

View reviewed changes

richardlau approved these changes Oct 24, 2017

View reviewed changes

refack approved these changes Oct 24, 2017

View reviewed changes

mscdex added the process Issues and PRs related to the process subsystem. label Oct 24, 2017

lpinca approved these changes Oct 25, 2017

View reviewed changes

gibfahn force-pushed the process-config branch from b967d53 to 45f3a06 Compare October 27, 2017 11:20

gibfahn force-pushed the process-config branch from 45f3a06 to 85daa2c Compare October 27, 2017 15:13

gibfahn closed this Oct 27, 2017

gibfahn force-pushed the process-config branch from 85daa2c to 502563b Compare October 27, 2017 15:17

gibfahn merged commit 502563b into nodejs:master Oct 27, 2017

gibfahn deleted the process-config branch October 27, 2017 15:18

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 30, 2017

tools: allow running test.py without configuring #16621

Merged

2 tasks

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

This was referenced May 13, 2024

[Snyk] Security upgrade eslint from 8.22.0 to 8.26.0 leonardoadame/node#818

Open

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#839

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skip test-process-config if no config.gypi #16436

test: skip test-process-config if no config.gypi #16436

gibfahn commented Oct 24, 2017

gibfahn commented Oct 24, 2017

richardlau Oct 24, 2017

refack Oct 24, 2017

gibfahn Oct 24, 2017

richardlau Oct 24, 2017

gibfahn Oct 24, 2017

refack Oct 24, 2017

refack left a comment

gibfahn commented Oct 25, 2017

refack commented Oct 27, 2017

test: skip test-process-config if no config.gypi #16436

test: skip test-process-config if no config.gypi #16436

Conversation

gibfahn commented Oct 24, 2017

Checklist

Affected core subsystem(s)

gibfahn commented Oct 24, 2017

richardlau Oct 24, 2017

Choose a reason for hiding this comment

refack Oct 24, 2017

Choose a reason for hiding this comment

gibfahn Oct 24, 2017

Choose a reason for hiding this comment

richardlau Oct 24, 2017

Choose a reason for hiding this comment

gibfahn Oct 24, 2017

Choose a reason for hiding this comment

refack Oct 24, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

gibfahn commented Oct 25, 2017

refack commented Oct 27, 2017