Skip to content

Fix race condition bug in test, and show assertion failure values#16037

Closed
kkwoker wants to merge 4 commits intonodejs:masterfrom
kkwoker:master
Closed

Fix race condition bug in test, and show assertion failure values#16037
kkwoker wants to merge 4 commits intonodejs:masterfrom
kkwoker:master

Conversation

@kkwoker
Copy link
Copy Markdown
Contributor

@kkwoker kkwoker commented Oct 6, 2017 •
edited by refack
Loading

Two changes:

  1. Fixed a bug in the test where the expected_result was caught in a race condition with the first two tests using promises.
  2. Cosmetic fixes for the test by removing the strings, making the assert return failure values.
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 dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Oct 6, 2017
@kkwoker kkwoker mentioned this pull request Oct 6, 2017
Closed
2 tasks
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Oct 7, 2017

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

refack
refack approved these changes Oct 7, 2017
View reviewed changes
Comment thread test/addons-napi/test_promise/test.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Oct 7, 2017

Choose a reason for hiding this comment

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

Suggestion: declare promise which is now a scoped var as const.

@refack
Copy link
Copy Markdown
Contributor

refack commented Oct 7, 2017

Except for lint error:

not ok 37 - /usr/home/iojs/build/workspace/node-test-linter/test/addons-napi/test_promise/test.js
  ---
  message: '''expected_result'' is never reassigned. Use ''const'' instead.'
  severity: error
  data:
    line: 11
    column: 7
    ruleId: prefer-const
  messages:
    - message: '''expected_result'' is never reassigned. Use ''const'' instead.'
      severity: error
      data:
        line: 24
        column: 7
        ruleId: prefer-const
  ...

CI is clean (CentOS5 failure is irrelevant)

@refack refack self-assigned this Oct 7, 2017
@refack
Copy link
Copy Markdown
Contributor

refack commented Oct 7, 2017

Welcome @kkwoker, and thank you for the contribution 🥇
Hope you find the time to follow up on this PR.

tniessen
tniessen reviewed Oct 7, 2017
View reviewed changes
Comment thread test/addons-napi/test_promise/test.js Outdated
Copy link
Copy Markdown
Member

@tniessen tniessen Oct 7, 2017

Choose a reason for hiding this comment

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

Nit: I would prefer not to have single lines separated by empty lines for assertions.

@kkwoker
Copy link
Copy Markdown
Contributor Author

kkwoker commented Oct 7, 2017

Hi, thank you for your help! I made the relevant fixes from your suggestions

@refack
Copy link
Copy Markdown
Contributor

refack commented Oct 7, 2017

@kkwoker thank you for following up.
Linter is now green - https://ci.nodejs.org/job/node-test-linter/12314/ ✔️

P.S. You had some unrelated commits from master in this PR so I rebased and force pushed. You can sync up by running git pull --rebase

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Oct 7, 2017

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

refack pushed a commit that referenced this pull request Oct 9, 2017
@kkwoker @refack
test: fix race condition in addon test
84579b1 
PR-URL: #16037
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Copy Markdown
Contributor

refack commented Oct 9, 2017

Landed in 84579b1

@kkwoker congratulations on being promoted from
image
to
image
🍾

@refack refack closed this Oct 9, 2017
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@kkwoker @MylesBorins
test: fix race condition in addon test
8f367bb 
PR-URL: #16037
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 11, 2017
Merged
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
@kkwoker @addaleax
test: fix race condition in addon test
6227d12 
PR-URL: nodejs/node#16037
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@digitalinfinity digitalinfinity mentioned this pull request Oct 18, 2017
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@kkwoker
test: fix race condition in addon test
f589058 
PR-URL: nodejs#16037
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@kkwoker @MylesBorins
test: fix race condition in addon test
53bd313 
Backport-PR-URL: #19447
PR-URL: #16037
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+2 more reviewers

@refack refack refack approved these changes

@evanlucas evanlucas evanlucas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. node-api Issues and PRs related to the Node-API. 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

@kkwoker @joyeecheung @refack @evanlucas @cjihrig @tniessen @Trott @nodejs-github-bot