Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromchalasr:patch_commandtester
Jun 16, 2016

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedMay 4, 2016
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRsymfony/symfony-docs#6623

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 callingCommandTester::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::setInputStream and assign it to the helperSet of the command, sort as all is done automatically in one call.

Depends on#18999

@chalasrchalasr changed the title[Console] Simplify user inputs simulation in the CommandTester[Console] Simplify simulation of user inputs in CommandTesterMay 4, 2016
@chalasrchalasrforce-pushed thepatch_commandtester branch 4 times, most recently fromad967f7 to63fbfd1CompareMay 5, 2016 18:42
{
$stream =fopen('php://memory','r+',false);

fputs($stream,implode("\n",$this->userInputs));

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

Copy link
MemberAuthor

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.
Copy link
Contributor

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
Copy link
Contributor

this should be documented somewhere

thanks for your effort 👍

@chalasr
Copy link
MemberAuthor

@OskarStark Thanks for the review.
I am waiting for the opinion of a maintainer before proposing the changes in the doc to make it in one shot.

OskarStark reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

@chalasr could you please make the test more complex to simulate several inputs in a row? Thanks!

@chalasrchalasrforce-pushed thepatch_commandtester branch 2 times, most recently fromdaab5d1 toe0be96fCompareMay 24, 2016 20:25
@chalasr
Copy link
MemberAuthor

chalasr commentedMay 24, 2016
edited
Loading

@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
Copy link
Contributor

I also fixed the@OskarStark line notes and started to write documentation.

Thank you for your effort 👍

chalasr reacted with thumbs up emoji

@chalasr
Copy link
MemberAuthor

chalasr commentedMay 26, 2016
edited
Loading

According to#17136, this will not work if the SymfonyStyle is used.
I'm working for find a clean solution (out of this PR), but making the SymfonyQuestionHelper out of the command HelperSet made interactive command testing very hard.
In a perfect world, the SymfonyQuestionHelper should be set in the helperSet and be accessible throughCommand::getHelper('question'). Actually the SymfonyStyle cannot access the Command and the Command cannot access the SymfonyStyle...

EDIT Should be fixed by#18902

@chalasr
Copy link
MemberAuthor

Docsubmitted.

*
* @return CommandTester
*/
publicfunctionsetUserInputs(array$inputs)
Copy link
Member

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
Copy link
MemberAuthor

I made the changes and updated the doc.

*
* @param array
*
* @return CommandTester
Copy link
Member

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.

OskarStark and chalasr reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I added a description.

@chalasrchalasrforce-pushed thepatch_commandtester branch 6 times, most recently fromb4768e0 to4c39bc8CompareJune 9, 2016 19:59
@chalasr
Copy link
MemberAuthor

chalasr commentedJun 9, 2016
edited
Loading

Rebased this on#18999 to see the benefit by testing commands using both QuestionHelper and SymfonyQuestionHelper (via SymfonyStyle, no need of an HelperSet).
Would be closed otherwise.

@chalasrchalasrforce-pushed thepatch_commandtester branch 2 times, most recently from438ca34 to0e64796CompareJune 9, 2016 22:46

return$this->statusCode =$this->command->run($this->input,$this->output);
}
/**
Copy link
Member

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.

Copy link
MemberAuthor

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

fabpot added a commit that referenced this pull requestJun 16, 2016
…(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
Copy link
Member

@chalasr Now that#18999 is merged, can you rebase this one?

return$this;
}

privatefunctiongetInputStream()
Copy link
Member

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.

Copy link
Member

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
Copy link
Member

Just noticed that the second commit messages contain all messages from squashed commits, can you remove them as they are useless? Thanks.

@chalasrchalasrforce-pushed thepatch_commandtester branch 3 times, most recently from769312f tob748435CompareJune 16, 2016 07:32
@chalasr
Copy link
MemberAuthor

Made the changes, rebased and cleaned the commit message.

Rename getInputStream() to getStream(), taking inputs as argumentMake createStream static, typehint  as array
@chalasr
Copy link
MemberAuthor

@fabpot As it takes the inputs as argument, I made thegetStream() method static.

return$this;
}

privatestaticfunctioncreateStream(array$inputs)
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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. 😕

Copy link
Member

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.

ogizanagi, chalasr, and alfredbez reacted with laugh emoji
@fabpot
Copy link
Member

Thank you@chalasr.

@fabpotfabpot merged commitc7ba38a intosymfony:masterJun 16, 2016
fabpot added a commit that referenced this pull requestJun 16, 2016
…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
@chalasrchalasr deleted the patch_commandtester branchJune 16, 2016 16:28
xabbuh added a commit to symfony/symfony-docs that referenced this pull requestJun 30, 2016
…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
@fabpotfabpot mentioned this pull requestOct 27, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@chalasr@OskarStark@javiereguiluz@fabpot@nicolas-grekas@xabbuh@ogizanagi@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp