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: fix arguments ordering for assertions to match the docs #23575

Closed
wants to merge 0 commits into from
Closed

test: fix arguments ordering for assertions to match the docs #23575

wants to merge 0 commits into from

Conversation

lirantal
Copy link
Member

@lirantal lirantal commented Oct 12, 2018 •
edited by BridgeAR
Loading

Expected

The test should make use of strictEqual() according to the docs with the first value being the actual, and second being the expected

Actual

The use of strictEqual() in the test uses the parameters backwards

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@addaleax addaleax added the module Issues and PRs related to the module subsystem. label Oct 12, 2018
gireeshpunathil
gireeshpunathil reviewed Oct 13, 2018
View reviewed changes
test/parallel/test-require-resolve.js Outdated
fixtures.path('a.js').toLowerCase(),
require.resolve(fixtures.path('a')).toLowerCase());
require.resolve(fixtures.path('a')).toLowerCase(),
fixtures.path('a.js').toLowerCase());
Copy link
Member

Choose a reason for hiding this comment

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

looks like the above 2 asserts are identical. Not an issue of this PR, but does any one know whether it is intentional vs. accidental?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch @gireeshpunathil, that change was introduced in 91a465c#diff-a00e02f229f04b6262da068e8df33ed5 and the version before that is b1506f7#diff-a00e02f229f04b6262da068e8df33ed5

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, one of them can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated and rebased 👍

thefourtheye
thefourtheye approved these changes Oct 13, 2018
View reviewed changes
test/parallel/test-require-resolve.js Outdated
fixtures.path('a.js').toLowerCase(),
require.resolve(fixtures.path('a')).toLowerCase());
require.resolve(fixtures.path('a')).toLowerCase(),
fixtures.path('a.js').toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, one of them can be removed.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
@lirantal @BridgeAR
test: fix arguments ordering for assertions to match the docs
c596bcc 
PR-URL: nodejs#23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@addaleax
Copy link
Member

Not sure what happened here, but there seem to be some odd commits in here … @lirantal Could you rebase this against master?

@lirantal lirantal closed this Oct 16, 2018
@lirantal
Copy link
Member Author

@addaleax looks like this PR has already landed in c596bcc#diff-a00e02f229f04b6262da068e8df33ed5

jasnell pushed a commit that referenced this pull request Oct 17, 2018
@lirantal @jasnell
test: fix arguments ordering for assertions to match the docs
d35965b 
PR-URL: #23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@lirantal @addaleax
test: fix arguments ordering for assertions to match the docs
80a78dc 
PR-URL: #23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@lirantal @MylesBorins
test: fix arguments ordering for assertions to match the docs
2a3ba01 
PR-URL: #23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@lirantal @rvagg
test: fix arguments ordering for assertions to match the docs
98915f1 
PR-URL: #23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@lirantal @MylesBorins
test: fix arguments ordering for assertions to match the docs
8f1cea6 
PR-URL: #23575
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. module Issues and PRs related to the module 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.
10 participants
@lirantal @addaleax @digitalinfinity @thefourtheye @cjihrig @gireeshpunathil @BridgeAR @trivikr @targos @nodejs-github-bot