Skip to content

test: check symbols in shared lib#18806

Closed
yhwang wants to merge 1 commit intonodejs:masterfrom
yhwang:sharedlib-test-fix-postmortem
Closed

test: check symbols in shared lib#18806
yhwang wants to merge 1 commit intonodejs:masterfrom
yhwang:sharedlib-test-fix-postmortem

Conversation

@yhwang
Copy link
Member

@yhwang yhwang commented Feb 15, 2018

When building the node with --shared option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 Feb 15, 2018
richardlau
richardlau reviewed Feb 16, 2018
View reviewed changes
test/parallel/test-postmortem-metadata.js Outdated
Copy link
Member

@richardlau richardlau Feb 16, 2018

Choose a reason for hiding this comment

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

I wonder if the logic for getting the shared lib instead of the stub executable is worth putting in the test/common/shared-lib-util.js being added in #18626. The platform specific logic could then be kept out of these changes.

Copy link
Member Author

@yhwang yhwang Feb 16, 2018

Choose a reason for hiding this comment

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

sure. that would be great! Since that PR is not landed yet. I will do that after the merge.

Copy link
Member Author

@yhwang yhwang Feb 16, 2018

Choose a reason for hiding this comment

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

@richardlau per your suggestion and the PR is landed, I moved the shared lib path logic into test/common/shared-lib-util.js

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from beedb23 to 39c2edf Compare February 16, 2018 17:35
cjihrig
cjihrig reviewed Feb 16, 2018
View reviewed changes
test/parallel/test-postmortem-metadata.js Outdated
Copy link
Contributor

@cjihrig cjihrig Feb 16, 2018

Choose a reason for hiding this comment

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

Why not just conditionally create args?

Copy link
Member Author

@yhwang yhwang Feb 16, 2018

Choose a reason for hiding this comment

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

sure! :-)

Copy link
Member Author

@yhwang yhwang Feb 16, 2018

Choose a reason for hiding this comment

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

it's done.

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 39c2edf to 79842cc Compare February 16, 2018 18:17
richardlau
richardlau reviewed Feb 16, 2018
View reviewed changes
test/common/shared-lib-util.js Outdated
Copy link
Member

@richardlau richardlau Feb 16, 2018

Choose a reason for hiding this comment

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

One too many get's 😁.

Copy link
Member Author

@yhwang yhwang Feb 16, 2018

Choose a reason for hiding this comment

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

oops....... thanks @richardlau . fixed

@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 79842cc to 0da2151 Compare February 16, 2018 19:39
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 17, 2018

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

@yhwang
Copy link
Member Author

yhwang commented Feb 19, 2018 •
edited
Loading

debian8-x64 failed on async-hooks/test-getaddrinforeqwrap test case. The change only modify test case parallel/test-postmortem-metadata. The failure should be irrelevant. async-hooks/test-getaddrinforeqwrap is not in flaky list. Let me kick off another CI.

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

@yhwang
Copy link
Member Author

yhwang commented Feb 19, 2018

previous CI passed the windows build and the second CI passed the linux test. I think it's green for this change.

@yhwang
test: check symbols in shared lib
01374fe 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: nodejs#18535

Signed-off-by: Yihong Wang <[email protected]>
@yhwang yhwang force-pushed the sharedlib-test-fix-postmortem branch from 0da2151 to 01374fe Compare February 20, 2018 22:36
@yhwang
Copy link
Member Author

yhwang commented Feb 20, 2018

rebased because of the conflict and new CI: https://ci.nodejs.org/job/node-test-commit/16393/

@addaleax
Copy link
Member

addaleax commented Feb 21, 2018

Landed in ab7c627

@addaleax addaleax closed this Feb 21, 2018
addaleax pushed a commit that referenced this pull request Feb 21, 2018
@yhwang @addaleax
test: check symbols in shared lib
ab7c627 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@yhwang yhwang deleted the sharedlib-test-fix-postmortem branch February 21, 2018 23:05
@yhwang
Copy link
Member Author

yhwang commented Feb 21, 2018

thanks @addaleax

MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
1d6b136 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
5bbf009 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@yhwang @MayaLekova
test: check symbols in shared lib
17a1691 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: nodejs#18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: nodejs#18806
Refs: nodejs#18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
903c07f 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
6221476 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
01f5015 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
@yhwang @MylesBorins
test: check symbols in shared lib
2f6866e 
When building the node with `--shared` option, we need
to verify the symbols in shared lib instead of executable.

Refs: #18535

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: #18806
Refs: #18535
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@hiroppy hiroppy hiroppy approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@yhwang @BridgeAR @addaleax @cjihrig @hiroppy @richardlau @gibfahn @MylesBorins @nodejs-github-bot