-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: increase coverage for internal/module.js #13673
Conversation
Tests should be fine but i was not able to run them locally. |
Both an answer and a nit: Add |
Ohh and P.S. 😉 |
75caabb
to
afe49c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add these changes in a separate file. I don't think we want to lose the things that are currently removed in this PR.
require(path.join(common.fixturesDir, 'internal-modules')), | ||
42 | ||
internalModule.stripShebang('#!bin/bash\naa'), | ||
'\naa' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like the following would be far more compact (and likely easier to maintain):
[
['', ''],
['aa', 'aa'],
['#!', ''],
['#!bin/bash', ''],
['#!bin/bash\naa', '\naa']
].forEach((i) => assert.strictEqual(internalModule.stripShebang(i[0]), i[1]));
@cjihrig I've restored the original test file which was intended to check the availability of the internal modules and moved my refactored tests into a new file. |
I've just seen the failing OSX test/build. Did i miss something? |
Don't worry, it's unrelated. |
PR-URL: #13673 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 83ebb6d |
@TamasHodi congradulations on landing your first contribution |
PR-URL: #13673 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13673 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add tests for the
stripShebang
function.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test internal modules