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

Prevent coveralls from running locally #3449

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@misteroneill
Member

misteroneill commented Jul 21, 2016

Description

The bash condition that previously existed in package.json to check for the TRAVIS env var was not working properly for everyone, causing grunt coveralls to run (and fail) locally.

Additionally, it would fail on Windows, since it was bash.

Specific Changes proposed

This moves that condition checking for the TRAVIS env var to JavaScript, where it now works reliably.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors
Prevent coveralls from running locally
The bash condition that previously existed in package.json was not
working properly in all environments. Additionally, it would fail on
Windows. This moves that condition to JavaScript, where it now works
reliably.
@@ -14,7 +14,7 @@
"homepage": "http://videojs.com",
"author": "Steve Heffernan",
"scripts": {
"test": "grunt test && if [ '$TRAVIS' ]; then grunt coveralls; fi;"

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Jul 21, 2016

Contributor

you could also just double quote and escape travis here like grunt test && if [ \"$TRAVIS\" ]; then grunt coveralls; fi;" but if were going for windows build compatibility then grunt is he better way. LGTM other than that consideration

@BrandonOCasey

BrandonOCasey Jul 21, 2016

Contributor

you could also just double quote and escape travis here like grunt test && if [ \"$TRAVIS\" ]; then grunt coveralls; fi;" but if were going for windows build compatibility then grunt is he better way. LGTM other than that consideration

This comment has been minimized.

@gkatsev

gkatsev Jul 21, 2016

Member

Yeah, we may as well and try to make it work on windows.

@gkatsev

gkatsev Jul 21, 2016

Member

Yeah, we may as well and try to make it work on windows.

This comment has been minimized.

@misteroneill

misteroneill Jul 21, 2016

Member

The fact that using " instead of ' would fix it suggests to me that sticking to JS instead of Bash is the way to go regardless of Windows support. 😄

@misteroneill

misteroneill Jul 21, 2016

Member

The fact that using " instead of ' would fix it suggests to me that sticking to JS instead of Bash is the way to go regardless of Windows support. 😄

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Jul 21, 2016

Contributor

yeah bash is a fickle beast

@BrandonOCasey

BrandonOCasey Jul 21, 2016

Contributor

yeah bash is a fickle beast

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 21, 2016

Member

LGTM

Member

gkatsev commented Jul 21, 2016

LGTM

@gkatsev gkatsev closed this in b270f58 Jul 21, 2016

@misteroneill misteroneill deleted the misteroneill:fix-npm-test branch Jul 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment