NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Console] Add SymfonyStyle::setInputStream() to ease command testing#18902

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

Jump to bottom

Closed

chalasr wants to merge1 commit intosymfony:masterfromchalasr:sfstyle_inputstream

Closed

[Console] Add SymfonyStyle::setInputStream() to ease command testing#18902

chalasr wants to merge1 commit intosymfony:masterfromchalasr:sfstyle_inputstream

Conversation

Copy link

Member

chalasr commentedMay 27, 2016•
edited
Loading

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	not yet

To make a command that usesSymfonyStyle::ask() able to be tested, I propose to add aSymfonyStyle::setInputStream(). I added a test that shows how to use it and depending on maintainer reactions, I'll quickly submit a doc PR.

Adding a setQuestionHelper as in#17136 doesn't make sense to me because if I have to test a command with the normal QuestionHelper, I would simply useMyCommand::getHelper('question')->ask() out of the SymfonyStyle IO.

This PR would makes#18710 even better (working for SymfonyStyle by simply doingSymfonyStyle::setInputStream($this->getHelper('question')->getInputStream()) from inside a command).

carsonbot added Status: Needs Review Console Feature labels

May 27, 2016

ogizanagi reviewed

May 27, 2016

View reviewed changes

src/Symfony/Component/Console/Style/SymfonyStyle.php Outdated

		*/
		publicfunctionsetInputStream($stream)
		{
		if (!is_resource($stream)) {

Copy link

Contributor

ogizanagiMay 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

@should beSymfony\Component\Console\Exception\InvalidArgumentException to conformSymfony\Component\Console\Helper\QuestionHelper (Should probably add the@throw in docblock too and same description as inQuestionHelper)

Copy link

MemberAuthor

chalasrMay 27, 2016

Choose a reason for hiding this comment

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

Thank's@ogizanagi

chalasr force-pushed thesfstyle_inputstream branch from2d3e78b to1ab89a1Compare

May 27, 2016 11:40

ogizanagi reviewed

May 27, 2016

View reviewed changes

src/Symfony/Component/Console/Style/SymfonyStyle.php Outdated

		*/
		publicfunctionsetInputStream($stream)
		{
		if (!is_resource($stream)) {

Copy link

Contributor

ogizanagiMay 27, 2016

Choose a reason for hiding this comment

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

- throw new InvalidArgumentException(sprintf('Input stream must be a valid resource, %s given', gettype($stream)));+ throw new InvalidArgumentException(sprintf('Input stream must be a valid resource, "%s" given', gettype($stream)));

(In Symfony, most exception messages use doubles quotes for values. At least it is used for FQCN, don't know about value's type).

Copy link

MemberAuthor

chalasrMay 27, 2016

Choose a reason for hiding this comment

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

I hesitated but It seems that types are unquoted when the method is not expecting a specific class instance so it should be ok. See theDI ParameterBag for instance.

Copy link

Member

xabbuhMay 29, 2016

Choose a reason for hiding this comment

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

Imo you should simply remove this check. The docblock already states that the method expects a resource (we never have checks like this).

Copy link

MemberAuthor

chalasrMay 29, 2016

Choose a reason for hiding this comment

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

Thank's@xabbuh

Copy link

Contributor

ogizanagi commentedMay 27, 2016

IMHO this is a far better solution. 😃

Now that we can test interactive inputs, I think you can add fixtures to test questions output interactively, as we discussed.

You can find some code samples you can take back/adapt in#14623 (comment) I used when working on the auto-gap feature.

ogizanagi reviewed

May 27, 2016

View reviewed changes

src/Symfony/Component/Console/Style/SymfonyStyle.php

		* Sets the input stream used by the SymfonyQuestionHelper.
		*
		* @param resource $stream
		*

Copy link

Contributor

ogizanagiMay 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

    /**     * Sets the input stream to read from when interacting with the user.     *     * This is mainly useful for testing purpose.     *     * @param resource $stream The input stream     *     * @throws InvalidArgumentException In case the stream is not a resource     */

(same as inQuestionHelper::setinputStream())

Copy link

MemberAuthor

chalasrMay 27, 2016

Choose a reason for hiding this comment

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

perfect :)

chalasr force-pushed thesfstyle_inputstream branch fromaf8c000 to3d2ed65Compare

May 27, 2016 16:20

chalasr mentioned this pull request

May 27, 2016

[Console] Simplify simulation of user inputs in CommandTester#18710

Merged

chalasr force-pushed thesfstyle_inputstream branch 3 times, most recently fromd292651 todfd22bbCompare

May 29, 2016 15:51

fabpot reviewed

May 29, 2016

View reviewed changes

...ymfony/Component/Console/Tests/Fixtures/Style/SymfonyStyle/command/interactive_command_1.php Outdated

		'How are you?',
		'Where do you come from?',
		);
		$inputs = ['Foo', 'Bar', 'Baz'];

Copy link

Member

fabpotMay 29, 2016

Choose a reason for hiding this comment

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

should bearray(...)

Copy link

MemberAuthor

chalasrMay 29, 2016

Choose a reason for hiding this comment

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

Fixed, thanks

chalasr force-pushed thesfstyle_inputstream branch from7b7eafd to5c14af7Compare

May 29, 2016 17:49

Copy link

MemberAuthor

chalasr commentedMay 29, 2016•
edited
Loading

@ogizanagi I added a test forSymfonyStyle::ask output, it works well on travis and my side, but the test breaks on appveyor... The output seems different than the expected one (say BinaryString, nothing more). So I rollback, it would be nice if you can take a look atthe test, maybe I forgot something.
If you don't see anything, I'll investigate on the same environment as appveyor and propose these additional tests in a next PR.

Copy link

Contributor

ogizanagi commentedMay 29, 2016•
edited
Loading

@chalasr : Indeed that's weird. Sorry but I can't see any issue with your code. 😄

The binary string mentioned is in fact an hexadecimal encoded string containing the proper output, except that it is decorated:

I've triggered myself a build on AppVeyor with a slightly different code in order to force the output not to be decorated, but unfortunately, the build failed, so I didn't get the result of the test.

Maybe you can try to change this line:

- $this->tester->execute(array());+ $this->tester->execute(array(), array('interactive' => true, 'decorated' => false));

The console component probably tried to see if the output can be decorated, and the AppVeyor's one can be, apparently.

Copy link

MemberAuthor

chalasr commentedMay 30, 2016

@ogizanagi I will try to add the colors in the output file, the result of your converter renders them ( tags rendered).

sstok reviewed

May 30, 2016

View reviewed changes

src/Symfony/Component/Console/Style/SymfonyStyle.php

		if (!$this->questionHelper) {
		$this->questionHelper =newSymfonyQuestionHelper();

		if ($this->inputStream) {

Copy link

Contributor

sstokMay 30, 2016

Choose a reason for hiding this comment

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

It should be possible to set after creating the SymfonyQuestionHelper instance.
What about detecting insetInputStream() if$this->questionHelper is not null and then callingsetInputStream() on the SymfonyQuestionHelper object?

Copy link

MemberAuthor

chalasrMay 30, 2016

Choose a reason for hiding this comment

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

Good catch@sstok, but I think it needs to keep an additional check in ask() because in most cases the helper will not be set when calling setInputStream, because it is set in ask()

Copy link

Contributor

sstokMay 30, 2016•
edited
Loading

Choose a reason for hiding this comment

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

True, that's actually what I thought (but forgot to clearly communicate) :)

One check (the current one) in askQuestion() to set it initially, and then in setInputStream() detect if questionHelper is already created and set the inputStream for the questionHelper instance (overwriting what was already set).

Copy link

Contributor

ogizanagi commentedMay 30, 2016•
edited
Loading

@chalasr ? That's what I meant by "the output is decorated" indeed. Thus you should disable the decoration by setting thedecorated flag to false as mentioned above :)

Copy link

MemberAuthor

chalasr commentedMay 30, 2016

Sorry I just misread you ^^, I give it a try and keep you informed.

chalasr force-pushed thesfstyle_inputstream branch 2 times, most recently fromda863e0 toaed1a8fCompare

May 30, 2016 12:08

Copy link

MemberAuthor

chalasr commentedMay 30, 2016•
edited
Loading

@ogizanagi Passed with decorated:false :)

So BTW I added a test forSymfonyStyle::ask output.

chalasr force-pushed thesfstyle_inputstream branch fromaed1a8f to1a53f43Compare

May 30, 2016 20:22

[Console] Add SymfonyStyle::setInputStream for command testing

d981fd3

Use Console InvalidArgumentException, test the exceptionUse better phpdoc blockFabbot fixesDon't check stream type in SymfonyStyle::setInputStreamTest output of an interactive command with questionsCS FixesAdd tests for SymfonyStyle::ask() outputAdd an additional check for SymfonyQuestionHelper::setInputStream

chalasr force-pushed thesfstyle_inputstream branch from1a53f43 tod981fd3Compare

May 30, 2016 21:38

chalasr mentioned this pull request

Jun 8, 2016

[Console] Centralize input stream in base Input class#18999

Merged

Copy link

MemberAuthor

chalasr commentedJun 8, 2016•
edited
Loading

Closed in favor of#18999 that makes this useless.

chalasr closed this

Jun 9, 2016

fabpot added a commit that referenced this pull request

Jun 16, 2016

feature#18999[Console] Centralize input stream in base Input class …

9759a35

…(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

chalasr deleted the sfstyle_inputstream branch

May 10, 2017 16:44

Labels

Console Feature Status: Needs Review

6 participants

Movatterモバイル変換

Uh oh!

[Console] Add SymfonyStyle::setInputStream() to ease command testing#18902

[Console] Add SymfonyStyle::setInputStream() to ease command testing#18902

Uh oh!

Conversation

chalasr commentedMay 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ogizanagiMay 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi commentedMay 27, 2016

Uh oh!

ogizanagiMay 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commentedMay 29, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ogizanagi commentedMay 29, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedMay 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstokMay 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi commentedMay 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedMay 30, 2016

Uh oh!

chalasr commentedMay 30, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedJun 8, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chalasr commentedMay 27, 2016•
edited
Loading

ogizanagiMay 27, 2016•
edited
Loading

ogizanagiMay 27, 2016•
edited
Loading

chalasr commentedMay 29, 2016•
edited
Loading

ogizanagi commentedMay 29, 2016•
edited
Loading

sstokMay 30, 2016•
edited
Loading

ogizanagi commentedMay 30, 2016•
edited
Loading

chalasr commentedMay 30, 2016•
edited
Loading

chalasr commentedJun 8, 2016•
edited
Loading