fix test-module-loading test case

[tarang9211]

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

[gibfahn]

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

Maybe something like this?

test: remove extra arg from test-module-loading

[tarang9211]

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]

Just modified the commit message also.

[jasnell]

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

[tarang9211]

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

[jasnell]

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

[tarang9211]

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]

You'd want,

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

[tarang9211]

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]

@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.

[tarang9211]

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

[jasnell]

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

[tarang9211]

@jasnell Do I break something :O ?

[gibfahn]

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

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

[tarang9211]

@gibfahn one more test left 😄

[gibfahn]

@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

[tarang9211]

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

[gibfahn]

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!

[tarang9211]

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

[gibfahn]

@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.