FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix test-module-loading test case by tarang9211 · Pull Request #11413 · nodejs/node · GitHub

/ node Public

fix test-module-loading test case - #11413

Closed
tarang9211 wants to merge 2 commits into
nodejs:masterfrom
tarang9211:test/fix-test-module-loading
Closed

fix test-module-loading test case#11413
tarang9211 wants to merge 2 commits into
nodejs:masterfrom
tarang9211:test/fix-test-module-loading

Conversation

tarang9211 commented Feb 16, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

First contribution to the node repo. This PR provides a small fix to the test-module-loading test case.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 16, 2017
tarang9211 changed the title eremoved extraneous filename parameter fix test-module-loading test case Feb 16, 2017
mscdex added the module Issues and PRs related to the module subsystem. label Feb 16, 2017

gibfahn commented Feb 16, 2017

Copy link
Copy Markdown
Member

Please could you format your commit message according to the Contributing Guidelines?

Maybe something like this?

test: remove extra arg from test-module-loading

Copy link
Copy Markdown
Contributor Author

One question I had was regarding this. I wanted to add a second argument (a reg exp) that matched the error message, but I'm not sure what the error message is.

tarang9211 force-pushed the test/fix-test-module-loading branch from 6e9ecad to d227bf8 Compare February 16, 2017 15:27

Copy link
Copy Markdown
Contributor Author

Just modified the commit message also.

jasnell commented Feb 16, 2017

Copy link
Copy Markdown
Member

@tarang9211 to find out the error message, simply remove the assert.throws() wrapper and run the test and see what error is reported :-)

Copy link
Copy Markdown
Contributor Author

@jasnell so basically just this function() { require('./utils') } ?

jasnell commented Feb 16, 2017

Copy link
Copy Markdown
Member

... just requires('./utils')

tarang9211 commented Feb 16, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

hey @jasnell thanks. here's a pastebin of the logs. Is it the string test name clashes or Cannot find module './utils' ?

If I add that as a second argument, the tests still fail.

jasnell commented Feb 16, 2017
edited
Loading

Copy link
Copy Markdown
Member

You'd want,

assert.throws(function() { require('./utils'); },
              /^Error: Cannot find module '\.\/utils'$/);

tarang9211 commented Feb 17, 2017
edited by gibfahn
Loading

Copy link
Copy Markdown
Contributor Author

Yup I tried that, too. I get the same output log as the pastebin above.
This is exactly what I have:

assert.throws(function() { require('./utils'); },
              /^Error: Cannot find module '.\/utils'$/);

The regex is appropriate as I got it to validate.

targos commented Feb 18, 2017

Copy link
Copy Markdown
Member

@tarang9211 The output log is normal, as long as you don't have a stack trace at the end. I tried running the test with your regexp and it works so I think you can push the change.

Copy link
Copy Markdown
Contributor Author

@targos sounds good, thanks for the update. Do have a look at the most recent commit in that case.

jasnell commented Feb 20, 2017

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Contributor Author

@jasnell Do I break something :O ?

gibfahn commented Feb 20, 2017

Copy link
Copy Markdown
Member

@tarang9211 looks like an infrastructure flake, let's try again:

CI 2: https://ci.nodejs.org/job/node-test-commit/8032/

Copy link
Copy Markdown
Contributor Author

@gibfahn one more test left 😄

gibfahn commented Feb 20, 2017

Copy link
Copy Markdown
Member

@tarang9211 If you click on the link you'll see everything passed. The test/arm failure in the GitHub UI is a reporting infra issue. See nodejs/build#572

Copy link
Copy Markdown
Contributor Author

@gibfahn Oh alright, thanks clears it up. So this is ready to be merged?

gibfahn self-assigned this Feb 21, 2017
gibfahn pushed a commit that referenced this pull request Feb 21, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

gibfahn commented Feb 21, 2017
edited
Loading

Copy link
Copy Markdown
Member

Landed in a4c3e31

I squashed the commits and modified the commit message, @tarang9211 in future you could try doing this yourself (not required but helpful!)

Thanks and congrats on your first PR to node!

gibfahn closed this Feb 21, 2017

Copy link
Copy Markdown
Contributor Author

@gibfahn sweet! So has this been merged into master, yet?

gibfahn commented Feb 21, 2017
edited
Loading

Copy link
Copy Markdown
Member

@tarang9211 Yes, if you look at the commit I posted above (a4c3e31) you'll see that it's in master. We don't merge commits in because that creates an extra merge commit (which isn't necessary).

So if you see the Landed in <commit sha> comment, you know the PR has landed.

addaleax pushed a commit that referenced this pull request Feb 22, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Also removes extraneous argument.

PR-URL: #11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins mentioned this pull request Mar 9, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL