-
-
Notifications
You must be signed in to change notification settings - Fork 616
Accept multiple path arguments #996
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
Conversation
bf93ca6 to
7e2f746
Compare
|
I personnally think this would be an awesome improvement, very usefull on CircleCI indeed to parallelize execution. |
| * @return string[] | ||
| */ | ||
| public function getPath() | ||
| public function getPaths() |
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.
Please also add getPath() with @deprecated tag for BC. It can return the first path given.
| parent::__construct($message); | ||
|
|
||
| $this->path = $path; | ||
| $this->paths = $paths; |
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.
For BC please ensure that $paths argument can be either a string or an array. Both must be supported for now.
$this->paths = (array) $paths;
Argh. Good catch :/ There is a way, though. You can switch back to instead of But it is surely better than waiting for 4.0, which is not even scheduled yet. |
|
@everzet but would it work for the circleCI features ? Btw, this would be a nightmare to implement, because you can have spaces inside path, so we would have to provide our own escaping inside this string. And requiring to escape spaces in the path to avoid considering the argument as a path list is a BC break for callers of Behat (and makes it much harder to use Behat by default), which is worse than breaking extensions IMO (especially given that extensions should generally not mess with arguments implemented by another one IMO) |
|
thus, if an extension relies on the argument directly, changing the content of the string from a path to a space-delimited list of escaped path would also break BC for the extension, as it would have to change the way it uses the string. |
|
:/ |
|
Here's Circle CI behaviour : https://circleci.com/docs/parallel-manual-setup/#using-configuration-files-modifier
I'm not sure how it'll react if behat expects it to be quoted, but I wouldn't recommend so. Well, that's too bad that the argument is already named What about bumping to 4.0.0 ? |
7e2f746 to
574b863
Compare
574b863 to
7dc66c8
Compare
|
@everzet I rebased on master and made WrongPathsException backwards compatible. It sounds like there isn't a good way to prevent breaking BC. I'm personally ok with waiting until 4.0, even if it takes a few years :) |
|
voice-over-sponge-bob-voice: 3 years later |
|
Hey, is anything else needed for this besides a rebase? What could I do to help? |
|
@pamil Do you have any plans when this will get merged/released? Cause it is also hindering my git hooks a little bit. I am using githooks as described in captainhook-git/captainhook#128 PS: Glad to see you maintaining Behat/Behat. |
|
Can you please target this to the 4.x branch? |
This PR adds the ability to pass multiple path arguments:
We use CircleCI to run a large suite of Behat tests across multiple docker containers. CircleCI can automatically balance tests across the containers to keep the run time as low as possible. The problem is, Circle assumes the test runner will accept multiple file arguments.
I'm working around this at the moment with a bash script and a Behat extension so I can invoke behat like
behat --feature apples.feature --feature pears.feature. I can't make an extension that directly changes thepathsargument to accept an array because other extensions expect a string.This is a breaking change, so it will need to wait for Behat 4.0.0.
Backwards (In)compatibility:
$input->getArgument('paths')or$input->setArgument('paths')and expecting a string will break.What do you think?