Skip to content
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

Fix test command when using docker #831

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Fix test command when using docker #831

merged 2 commits into from
Aug 13, 2018

Conversation

BBlackwo
Copy link
Contributor

@BBlackwo BBlackwo commented Aug 7, 2018

  • backstop test --docker was actually running reference instead. Fixed that and did a bit of refactoring so there's less copied and pasted code.
  • If you have spaces in a command it now adds quotes around it when passing through to Docker. E.g. backstop test --docker --filter="some test".
  • Now --docker will work on window as I'm using ${process.cwd()} instead of $(pwd) in the docker command.
  • Also added a CONTRIBUTING.md guide. It's a start. I couldn't figure out what commands to run and such to test my changes. Had to dig through the package.json > scripts. I think it could help others too.

Was running reference instead of test when passing --docker flag
@BBlackwo BBlackwo changed the title Fix test command using docker Fix test command when using docker Aug 7, 2018
@BBlackwo
Copy link
Contributor Author

Any chance you can merge this one in @garris? It's been sitting around for almost a week now.

@brendonbribeiro
Copy link
Contributor

@BBlackwo I'm having some trouble running with docker.

if (config.args.docker) {
    const passAlongArgs = process.argv
                  .slice(3)
                  .join('" "') // in case of spaces in a command
                  .replace(/--docker/, '--moby');

And in my case, i don't want to use --docker, i want set this on my configuration file.
I know that this have nothing to do with your code, but maybe you could give the user the choice of running with --docker or set it on configuration file.

What do you think?

Copy link
Owner

@garris garris left a comment

Choose a reason for hiding this comment

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

Thank you for the PR — this is a nice refactor. My only comment is around the contributing doc — there are some notes in the readme for contributing— maybe we can just tell the user to go here...
https://github.com/garris/BackstopJS/blob/master/README.md#developing-bug-fixing-contributing

I am planning to release the next version on the 24th. This change will be in for that one.

Cheers.

@garris garris merged commit fe2d22d into garris:master Aug 13, 2018
@garris
Copy link
Owner

garris commented Aug 13, 2018

@BBlackwo Just occurred to me -- you're adding quotes to docker args in this PR... What happens if the user included these quotes in the original argument? 👉 e.g. --filter="myScenarioName"

https://github.com/garris/BackstopJS/pull/831/files#diff-0745f5ef38ef41d5c870fa5515ddfd83R7

@BBlackwo
Copy link
Contributor Author

BBlackwo commented Aug 13, 2018

@brendonbarreto being able to set it on the config file sounds reasonable. Maybe you can submit a PR for that :)

@garris thanks for the merge. Sorry I didn't see that section in the readme.

Also with the quotes, before my change if you ran --filter="My Scenario Name" the quotes would be stripped off when running the docker command. My change always puts them in case of spaces. The process.argv array didn't include the quotes in the arg, but just treated each arg as a string.

@BBlackwo
Copy link
Contributor Author

Added it also to the table of contents in #838

@BBlackwo BBlackwo deleted the fix/docker-test-blackwoodb branch August 13, 2018 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants