FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: skip test-process-config if no config.gypi by gibfahn · Pull Request #16436 · nodejs/node · GitHub

/ node Public

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

Merged
gibfahn merged 0 commit into
nodejs:masterfrom
gibfahn:process-config
Oct 27, 2017
ConversationCommits (0)ChecksFiles changed
Merged

test: skip test-process-config if no config.gypi#16436
gibfahn merged 0 commit into
nodejs:masterfrom
gibfahn:process-config

Conversation

gibfahn commented Oct 24, 2017

Copy link
Copy Markdown
Member

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 added the test Issues and PRs related to the tests. label Oct 24, 2017

gibfahn commented Oct 24, 2017

Copy link
Copy Markdown
Member Author

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

richardlau reviewed Oct 24, 2017
View reviewed changes
Comment thread test/parallel/test-process-config.js Outdated

richardlau Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are these necessary?

refack Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

gibfahn Oct 24, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

richardlau Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

gibfahn Oct 24, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Oct 24, 2017
View reviewed changes
Comment thread test/parallel/test-process-config.js Outdated

refack Oct 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Oct 24, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🏆

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

gibfahn commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

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

gibfahn closed this Oct 27, 2017
gibfahn merged commit 502563b into nodejs:master Oct 27, 2017
gibfahn deleted the process-config branch October 27, 2017 15:18

refack commented Oct 27, 2017

Copy link
Copy Markdown
Contributor

Follow up (just found this up):

node/tools/test.py

Line 941 in 22882d4

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

jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn mentioned this pull request Oct 30, 2017
Merged
2 tasks
gibfahn added a commit that referenced this pull request Oct 30, 2017
test: skip test-process-config if no config.gypi
9afc9bc 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn added a commit that referenced this pull request Oct 30, 2017
test: skip test-process-config if no config.gypi
d1ca3b9 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn added a commit that referenced this pull request Oct 31, 2017
test: skip test-process-config if no config.gypi
525700b 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
test: skip test-process-config if no config.gypi
68b1131 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
test: skip test-process-config if no config.gypi
add0859 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
test: skip test-process-config if no config.gypi
8cc5d5f 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn added a commit to gibfahn/node that referenced this pull request Nov 20, 2017
tools: allow running test.py without configuring
ad1967d 
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 <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
test: skip test-process-config if no config.gypi
075f1b7 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
test: skip test-process-config if no config.gypi
7d93da5 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
test: skip test-process-config if no config.gypi
1137bf5 
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 <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
tools: allow running test.py without configuring
4954eef 
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 <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
gibfahn added a commit that referenced this pull request Dec 19, 2017
tools: allow running test.py without configuring
92e0e2e 
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 <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
gibfahn added a commit that referenced this pull request Dec 20, 2017
tools: allow running test.py without configuring
7658eec 
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 <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
This was referenced May 13, 2024
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
richardlau richardlau approved these changes
gireeshpunathil gireeshpunathil approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL