Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Simplify simulation of user inputs in CommandTester#18710
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ad967f7 to63fbfd1Compare| { | ||
| $stream =fopen('php://memory','r+',false); | ||
| fputs($stream,implode("\n",$this->userInputs)); |
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.
I think we use fwrite instead of fputs
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.
I tried to follow thishttp://symfony.com/doc/current/components/console/helpers/questionhelper.html#testing-a-command-that-expects-input as possible, please let me know if you still think that I should use fwrite.
| } | ||
| /** | ||
| * Sets the faked user inputs. |
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.
would remove the wordfaked
OskarStark commentedMay 24, 2016
this should be documented somewhere thanks for your effort 👍 |
chalasr commentedMay 24, 2016
@OskarStark Thanks for the review. |
javiereguiluz commentedMay 24, 2016
@chalasr could you please make the test more complex to simulate several inputs in a row? Thanks! |
daab5d1 toe0be96fComparechalasr commentedMay 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@javiereguiluz Of course, I added a failure test (3 questions for only 2 inputs) and made the first taking 3 questions for 3 inputs. I also fixed the@OskarStark line notes and started to write documentation. |
OskarStark commentedMay 24, 2016
Thank you for your effort 👍 |
chalasr commentedMay 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
According to#17136, this will not work if the SymfonyStyle is used. EDIT Should be fixed by#18902 |
chalasr commentedMay 28, 2016
Docsubmitted. |
| * | ||
| * @return CommandTester | ||
| */ | ||
| publicfunctionsetUserInputs(array$inputs) |
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.
I would have named the methodsetInputs() instead.
chalasr commentedJun 2, 2016
I made the changes and updated the doc. |
| * | ||
| * @param array | ||
| * | ||
| * @return CommandTester |
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.
Imo this docblock as it is now is quite useless and could be removed as well (input types and the return type can be inferred by IDEs from the method itself) and the description doesn't add any value to the user apart from what the method name already states. Though explaining what$inputs exactly is expected to be would be something that justified its presence.
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.
I added a description.
b4768e0 to4c39bc8Comparechalasr commentedJun 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Rebased this on#18999 to see the benefit by testing commands using both QuestionHelper and SymfonyQuestionHelper (via SymfonyStyle, no need of an HelperSet). |
438ca34 to0e64796Compare| return$this->statusCode =$this->command->run($this->input,$this->output); | ||
| } | ||
| /** |
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.
There is an indentation problem here for this method.
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.
Fixed. The changes of this PR are separated in69dd333
…(chalasr)This PR was merged into the 3.2-dev branch.Discussion----------[Console] Centralize input stream in base Input class| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#10844| License | MIT| Doc PR | not yetActually we have two classes that have an `inputStream` property with its getter+setter:the QuestionHelper ('question' helper) and the SymfonyQuestionHelper (used via SymfonyStyle, that inherits the inputStream from the QuestionHelper class).This situation makes command testing really hard.ATM we can't test a command that uses `SymfonyStyle::ask` without ugly hacks, and we can't find a generic way to set the inputs passed to a command from the CommandTester, as it need to be done on a specific helper that may not be the one used by the command (SymfonyStyle case).What I propose here is to add a `stream` property (and its getter+setter) to the abstract `Symfony\Component\Console\Input\Input` class.For now I just made the two helpers setting their `inputStream` from `Input::getStream`, as both QuestionHelper and SymfonyQuestionHelper classes have an `ask` method that takes the command Input as argument.In a next time (4.0), we could remove the `getInputStream` and `setInputStream` methods from the QuestionHelper class (this only deprecates them in favor of Input `setStream` and `getStream`).This would close PR#18902 (trying to make interactive command testing with SymfonyStyle easier).This would also make PR#18710 widely better by setting the input stream in a generic way (working for both helpers without caring about if they are used and which one is used).Please give me your opinions.Commits-------54d3d63 Centralize input stream in base Input class
fabpot commentedJun 16, 2016
| return$this; | ||
| } | ||
| privatefunctiongetInputStream() |
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.
I would name itcreateInputStream() and pass$this->inputs as an argument.
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.
I would then rename it tocreateStream()
fabpot commentedJun 16, 2016
Just noticed that the second commit messages contain all messages from squashed commits, can you remove them as they are useless? Thanks. |
769312f tob748435Comparechalasr commentedJun 16, 2016
Made the changes, rebased and cleaned the commit message. |
Rename getInputStream() to getStream(), taking inputs as argumentMake createStream static, typehint as array
chalasr commentedJun 16, 2016
@fabpot As it takes the inputs as argument, I made the |
| return$this; | ||
| } | ||
| privatestaticfunctioncreateStream(array$inputs) |
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.
What's the benefit of marking it public? I would have kept it private.
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.
@fabpot : Did you meanstatic ?
I guess it makes sense, as this method looks like a utility one.
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.
No, I meant public. I think it should be private, I don't see why an end user would have to use this.
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.
@fabpot : But thecreateStream method is actuallyprivate. Notpublic. 😕
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.
lol, sorry about the confusion.
fabpot commentedJun 16, 2016
Thank you@chalasr. |
…dTester (chalasr)This PR was merged into the 3.2-dev branch.Discussion----------[Console] Simplify simulation of user inputs in CommandTester| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR |symfony/symfony-docs#6623After@javiereguiluz pointed it in#17470, I open this PR to simplify the simulation of user inputs for testing a Command.It would be done by calling `CommandTester::setUserInputs()` with an array of inputs as argument, and so make the CommandTester creating an input stream from the inputs set by the developer, then call `QuestionHelper::setInputStream` and assign it to the helperSet of the command, sort as all is done automatically in one call.Depends on#18999Commits-------c7ba38a [Console] Set user inputs from CommandTester
…ing user inputs (chalasr)This PR was merged into the master branch.Discussion----------[Console] Adapt doc for easier testing of commands needing user inputsDoc-PR forsymfony/symfony#18710.This one eases testing of commands needing user interactions by simulating them from the CommandTester directly, by creating a input stream from the ones set by the developer.Commits-------26fdbe0 [Console] Adapt doc for easier testing of commands needing user inputs
Uh oh!
There was an error while loading.Please reload this page.
After@javiereguiluz pointed it in#17470, I open this PR to simplify the simulation of user inputs for testing a Command.
It would be done by calling
CommandTester::setUserInputs()with an array of inputs as argument, and so make the CommandTester creating an input stream from the inputs set by the developer, then callQuestionHelper::setInputStreamand assign it to the helperSet of the command, sort as all is done automatically in one call.Depends on#18999