Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: skip postmortem metadata test when nm fails #19107

Closed
wants to merge 1 commit into from
Closed

test: skip postmortem metadata test when nm fails #19107

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

On Windows with msys installation, nm is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

cc @cjihrig

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 3, 2018
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13469/

lpinca
lpinca reviewed Mar 3, 2018
View reviewed changes
test/parallel/test-postmortem-metadata.js Outdated
@@ -26,6 +26,11 @@ const nm = spawnSync('nm', args);
if (nm.error && nm.error.errno === 'ENOENT')
common.skip('nm not found on system');

const stderr = nm.stderr.toString();
if (nm.stderr.toString().length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can't stderr be used here instead of repeating nm.stderr.toString()?

Copy link
Member Author

@joyeecheung joyeecheung Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@lpinca oops, somehow I forgot to replace this...thanks

Copy link
Member

Choose a reason for hiding this comment

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

Ping @joyeecheung

@joyeecheung
test: skip postmortem metadata test when nm fails
783c037 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13552/

@joyeecheung
Copy link
Member Author

Newi CI: https://ci.nodejs.org/job/node-test-pull-request/13566/

@joyeecheung
Copy link
Member Author

Landed in a6c14b2, thanks!

@joyeecheung joyeecheung closed this Mar 7, 2018
joyeecheung added a commit that referenced this pull request Mar 7, 2018
@joyeecheung
test: skip postmortem metadata test when nm fails
a6c14b2 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

PR-URL: #19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 17, 2018
@joyeecheung @targos
test: skip postmortem metadata test when nm fails
a08c857 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

PR-URL: #19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@joyeecheung @MylesBorins
test: skip postmortem metadata test when nm fails
46b5915 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

PR-URL: #19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
test: skip postmortem metadata test when nm fails
0b5df1c 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

PR-URL: nodejs#19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
@joyeecheung @jasnell
test: skip postmortem metadata test when nm fails
d875750 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

PR-URL: nodejs#19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
@joyeecheung @MylesBorins
test: skip postmortem metadata test when nm fails
d3a10c7 
On Windows with msys installation, `nm` is available but
it is not able to grab symbols from the Windows build.
Skipping the test if nm outputs anything to stderr fixes that.

Backport-PR-URL: #22380
PR-URL: #19107
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@mmarchini mmarchini mmarchini approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
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
@joyeecheung @bnoordhuis @jasnell @lpinca @cjihrig @mmarchini @BridgeAR @nodejs-github-bot