Skip to content

Test local commands #56

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

Closed
wants to merge 5 commits into from
Closed

Test local commands #56

wants to merge 5 commits into from

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Jul 21, 2016

Reference #52 (comment), hope this works…

@ghost ghost added the CLA Signed label Jul 21, 2016
@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

Not sure what's happening on Travis, I don't have access to the logs. @gaearon could you check?

@ghost ghost added the CLA Signed label Jul 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

This is because both argv checks assume there is only one argument. So you can't pass both --debug-template and --smoke-test. Replacing the comparisons with indexOf in both places should do the trick.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

Sorry, where do I pass --debug-template?

@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

It is being passed from npm start in local package. (I forgot to do this in that PR and did it in follow up commit).

This flag is read in DEV Webpack config. But there is another flag (for smoke test) read in start.js.

The smoke test flag ensures process terminates. (This is why your PR fails. Smoke test flag is tested for exact match with argv but there are two arguments now. indexOf check should fix it.)

@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

The problem is in the comparison. It compares '--debug-template --smoke-test' === 'smoke-test' and it fails. It should instead use indexOf. Search for process.argv and you'll see where it happens.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

Ahh, I got what you mean now, thanks. Should be fixed in the last commit, let's see if Travis passes… ✨

@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

Now that I think of it we probably should search argv array itself. Also need to do it in both places.

@vjeux
Copy link
Contributor

vjeux commented Jul 21, 2016

Since it's starting to be complicated parsing of arguments, should probably use minimist: https://github.com/facebookincubator/create-react-app/blob/master/global-cli/index.js#L43

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

Should just be a case of running process.argv.some((item) => item.indexOf('--smoke-test') > -1)), not sure it's worth introducing a dependency for that…

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

both places

Where's the other place?

@ghost ghost added the CLA Signed label Jul 21, 2016
@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

This took way longer than anticipated, but should finally be done hopefully!

@gaearon
Copy link
Contributor

gaearon commented Jul 21, 2016

😄

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

God dammit, merge conflict

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 21, 2016

Merge?!

npm run build

# Check for expected output
test -e build/*.html || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the exit 1. We use set -e which bails on any commands that returns non 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added those in the first place, I didn’t realize that.
@mxstbr You can kill them throughout this script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@ghost ghost added the CLA Signed label Jul 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

Got it, thanks. I’ll rebase this new because there’s another conflict 😄

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 22, 2016

Thanks! Sorry this took that long 🙏

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

This is in now:

49e2fb5
9317d10
ce9fdca
c7bc89c

@gaearon gaearon closed this Jul 22, 2016
@gaearon gaearon deleted the test-local branch July 22, 2016 12:16
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants