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

chalasr · 2016-05-27T11:18:02Z

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 uses SymfonyStyle::ask() able to be tested, I propose to add a SymfonyStyle::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 use MyCommand::getHelper('question')->ask() out of the SymfonyStyle IO.

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

ogizanagi · 2016-05-27T11:26:50Z

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

+    public function setInputStream($stream)
+    {
+        if (!is_resource($stream)) {
+            throw new \InvalidArgumentException(sprintf('The stream must be a valid resource, %s given'), gettype($stream));


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

Thank's @ogizanagi

ogizanagi · 2016-05-27T11:47:00Z

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

+    public function setInputStream($stream)
+    {
+        if (!is_resource($stream)) {
+            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))); + 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).

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 the DI ParameterBag for instance.

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

Thank's @xabbuh

ogizanagi · 2016-05-27T11:50:08Z

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 · 2016-05-27T11:51:43Z

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

+     *
+     * @param resource $stream
+     *
+     * @throws InvalidArgumentException If the given stream is not a resource


/** * 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 in QuestionHelper::setinputStream())

fabpot · 2016-05-29T16:15:07Z

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

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


should be array(...)

Fixed, thanks

chalasr · 2016-05-29T19:07:26Z

@ogizanagi I added a test for SymfonyStyle::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 at the 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.

ogizanagi · 2016-05-29T20:07:15Z

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

chalasr · 2016-05-30T07:10:38Z

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

sstok · 2016-05-30T07:15:07Z

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

@@ -348,6 +349,10 @@ public function askQuestion(Question $question)

        if (!$this->questionHelper) {
            $this->questionHelper = new SymfonyQuestionHelper();
+
+            if ($this->inputStream) {


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

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

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

ogizanagi · 2016-05-30T07:16:38Z

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

chalasr · 2016-05-30T07:22:40Z

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

chalasr · 2016-05-30T12:19:35Z

@ogizanagi Passed with decorated:false :)

So BTW I added a test for SymfonyStyle::ask output.

Use Console InvalidArgumentException, test the exception Use better phpdoc block Fabbot fixes Don't check stream type in SymfonyStyle::setInputStream Test output of an interactive command with questions CS Fixes Add tests for SymfonyStyle::ask() output Add an additional check for SymfonyQuestionHelper::setInputStream

chalasr · 2016-06-08T19:53:21Z

Closed in favor of #18999 that makes this useless.

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

carsonbot added Status: Needs Review Console Feature labels May 27, 2016

ogizanagi reviewed May 27, 2016
View reviewed changes

chalasr force-pushed the sfstyle_inputstream branch from 2d3e78b to 1ab89a1 Compare May 27, 2016 11:40

ogizanagi reviewed May 27, 2016
View reviewed changes

chalasr force-pushed the sfstyle_inputstream branch from af8c000 to 3d2ed65 Compare 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 the sfstyle_inputstream branch 3 times, most recently from d292651 to dfd22bb Compare May 29, 2016 15:51

fabpot reviewed May 29, 2016
View reviewed changes

chalasr force-pushed the sfstyle_inputstream branch from 7b7eafd to 5c14af7 Compare May 29, 2016 17:49

sstok reviewed May 30, 2016
View reviewed changes

chalasr force-pushed the sfstyle_inputstream branch 2 times, most recently from da863e0 to aed1a8f Compare May 30, 2016 12:08

chalasr force-pushed the sfstyle_inputstream branch from aed1a8f to 1a53f43 Compare May 30, 2016 20:22

chalasr force-pushed the sfstyle_inputstream branch from 1a53f43 to d981fd3 Compare May 30, 2016 21:38

chalasr mentioned this pull request Jun 8, 2016

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

Merged

chalasr closed this Jun 9, 2016

chalasr deleted the sfstyle_inputstream branch May 10, 2017 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chalasr commented May 27, 2016 •

edited

Loading

ogizanagi May 27, 2016 •

edited

Loading

chalasr May 27, 2016

ogizanagi May 27, 2016

chalasr May 27, 2016

xabbuh May 29, 2016

chalasr May 29, 2016

ogizanagi commented May 27, 2016

ogizanagi May 27, 2016 •

edited

Loading

chalasr May 27, 2016

fabpot May 29, 2016

chalasr May 29, 2016

chalasr commented May 29, 2016 •

edited

Loading

ogizanagi commented May 29, 2016 •

edited

Loading

chalasr commented May 30, 2016

sstok May 30, 2016

chalasr May 30, 2016

sstok May 30, 2016 •

edited

Loading

ogizanagi commented May 30, 2016 •

edited

Loading

chalasr commented May 30, 2016

chalasr commented May 30, 2016 •

edited

Loading

chalasr commented Jun 8, 2016 •

edited

Loading

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

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

Conversation

chalasr commented May 27, 2016 • edited Loading

ogizanagi May 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented May 27, 2016

ogizanagi May 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented May 29, 2016 • edited Loading

ogizanagi commented May 29, 2016 • edited Loading

chalasr commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok May 30, 2016 • edited Loading

Choose a reason for hiding this comment

ogizanagi commented May 30, 2016 • edited Loading

chalasr commented May 30, 2016

chalasr commented May 30, 2016 • edited Loading

chalasr commented Jun 8, 2016 • edited Loading

chalasr commented May 27, 2016 •

edited

Loading

ogizanagi May 27, 2016 •

edited

Loading

ogizanagi May 27, 2016 •

edited

Loading

chalasr commented May 29, 2016 •

edited

Loading

ogizanagi commented May 29, 2016 •

edited

Loading

sstok May 30, 2016 •

edited

Loading

ogizanagi commented May 30, 2016 •

edited

Loading

chalasr commented May 30, 2016 •

edited

Loading

chalasr commented Jun 8, 2016 •

edited

Loading