fix test-module-loading test case#11413
Conversation
|
Please could you format your commit message according to the Contributing Guidelines? Maybe something like this? |
|
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. |
6e9ecad to
d227bf8
Compare
|
Just modified the commit message also. |
|
@tarang9211 to find out the error message, simply remove the |
|
@jasnell so basically just this |
|
... just |
|
You'd want, |
|
Yup I tried that, too. I get the same output log as the pastebin above. assert.throws(function() { require('./utils'); },
/^Error: Cannot find module '.\/utils'$/);The regex is appropriate as I got it to validate. |
|
@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. |
|
@targos sounds good, thanks for the update. Do have a look at the most recent commit in that case. |
|
@jasnell Do I break something :O ? |
|
@tarang9211 looks like an infrastructure flake, let's try again: |
|
@gibfahn one more test left 😄 |
|
@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 |
|
@gibfahn Oh alright, thanks clears it up. So this is ready to be merged? |
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>
|
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 sweet! So has this been merged into master, yet? |
|
@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 |
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>
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>
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>
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>
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>
First contribution to the node repo. This PR provides a small fix to the
test-module-loadingtest case.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes