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.

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

test, module: current directory gets priority for local module lookup #20545

Closed
wants to merge 8 commits into from

Conversation

RakshithNM
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 5, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Tiny nit: Can you separate the // from Current by a single space instead of two.

@mscdex
Copy link
Contributor

mscdex commented May 5, 2018

I think the prefix should just be test: since no changes to the actual module system are being made here?

@RakshithNM
Copy link
Contributor Author

@cjihrig yeah ok, will update

@RakshithNM
Copy link
Contributor Author

@mscdex ah ok, now i think so too.

@trivikr
Copy link
Member

trivikr commented May 5, 2018

@RakshithNM Looks like your Outlook email ID is linked with the commits, and is not registered with Github. Kindly either register it or update your Github email ID in the config by following the instructions in Step 1

If you choose the latter, you can squash the commits so that your updated Github email ID is associated with the new commit.

@Trott
Copy link
Member

Trott commented May 5, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
@RakshithNM
Copy link
Contributor Author

@trivikr ah yep, i have now verified my email

@Trott
Copy link
Member

Trott commented May 6, 2018

Can be fixed by whoever lands this, but IMO the commit message makes it sound like this is a functionality change when this is a test refactor. I'd propose something like this:

test: display values in AssertionErrors

In test-module-relative-lookup, change assert.strictEqual() calls so
that values are displayed when an AssertionError occurs.

rakshith_bellare@outlook.com added 2 commits May 6, 2018 19:42
In test-module-relative-lookup, change assert.strictEqual() calls so that values are displayed when an AssertionError occurs.
@RakshithNM
Copy link
Contributor Author

@Trott ah ok, i have ammended the commit message

@trivikr
Copy link
Member

trivikr commented May 6, 2018

@RakshithNM The author hasn't been updated in the commits
Here's an answer on StackOverflow on how to update author of existing commits

RakshithNM and others added 2 commits May 6, 2018 21:44
In test-module-relative-lookup, change assert.strictEqual() calls so that values are displayed when an AssertionError occurs.
@RakshithNM
Copy link
Contributor Author

@trivikr thanks for the link, i have now updated the author.

@addaleax
Copy link
Member

addaleax commented May 14, 2018

Landed in 745463a 🎉 🎉

@RakshithNM Just so you know, these merge conflicts are actually a bit of an issue – most of the time they can be ironed out while landing, but for the next PRs, using git rebase rather than git merge (or git pull --rebase rather than git pull) would help a bit :)

@addaleax addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
In test-module-relative-lookup, change assert.strictEqual()
calls so that values are displayed when an AssertionError occurs.

PR-URL: #20545
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@RakshithNM
Copy link
Contributor Author

@addaleax thank you :) I will keep that in mind moving forward.

addaleax pushed a commit that referenced this pull request May 14, 2018
In test-module-relative-lookup, change assert.strictEqual()
calls so that values are displayed when an AssertionError occurs.

PR-URL: #20545
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax addaleax mentioned this pull request May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants