-
Notifications
You must be signed in to change notification settings - Fork 243
fix(args): make args conversion stateless #85
fix(args): make args conversion stateless #85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 34 34
=====================================
Hits 34 34
Continue to review full report at Codecov.
|
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.
Thank you for contributing this! Could you add yourself to the contributors table?
test(`converts windows-style env variable usage for linux`, () => { | ||
isWindowsMock.__mock.returnValue = false | ||
expect(commandConvert('%test%')).toBe('$test') | ||
}) | ||
|
||
test(`leaves variable unchanged when using correct operating system`, () => { | ||
isWindowsMock.__mock.returnValue = true | ||
isWindowsMock.__mock.returnValue = false |
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.
Thank you!
src/command.test.js
Outdated
|
||
test(`is stateless`, () => { | ||
// this test prevents falling into regexp traps like this: | ||
// http://bit.ly/1zWb3tn |
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 use the full URL instead? http://stackoverflow.com/a/1520853/971592
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.
Sure! Tried to use http://stackoverflow.com/questions/1520800/why-regexp-with-global-flag-in-javascript-give-wrong-results/1520853#1520853 but it was too long for the 80 chars limit.
d09bb9d
to
e94d08d
Compare
All done. |
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.
Awesome! Thanks!
Hello.
I was starting to make a contribution to cross-env and found this test. It looks like it is buggy.
isWindowsMock
is set totrue
, but the test expects$test
not to be converted. And it passes! 😮I looked into it and discovered this interesting fact about
RegExp.prototype.exec
: it's not stateless when used with theg
flag! That's what is making the test pass.So I added this test:
It does fail on
master
. To fix the problem, I have removed theg
flag from the regexps incommand.js
. It is not useful anyway becauseexec
is only called once.Thanks for making cross-env, and for your consideration.