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] add setInputs to ApplicationTester and share some code#24819
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
[Console] add setInputs to ApplicationTester and share some code#24819
Uh oh!
There was an error while loading.Please reload this page.
Conversation
be6fbc2 todbbf3fbCompare
chalasr left a comment
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.
here is a test case
diff --git a/src/Symfony/Component/Console/Tests/Tester/ApplicationTesterTest.php b/src/Symfony/Component/Console/Tests/Tester/ApplicationTesterTest.phpindex 062d414..0cc002d 100644--- a/src/Symfony/Component/Console/Tests/Tester/ApplicationTesterTest.php+++ b/src/Symfony/Component/Console/Tests/Tester/ApplicationTesterTest.php@@ -13,7 +13,9 @@ namespace Symfony\Component\Console\Tests\Tester; use PHPUnit\Framework\TestCase; use Symfony\Component\Console\Application;+use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Output\Output;+use Symfony\Component\Console\Question\Question; use Symfony\Component\Console\Tester\ApplicationTester; class ApplicationTesterTest extends TestCase@@ -65,6 +67,22 @@ class ApplicationTesterTest extends TestCase $this->assertEquals('foo'.PHP_EOL, $this->tester->getDisplay(), '->getDisplay() returns the display of the last execution'); }+ public function testSetInputs()+ {+ $this->application->register('foo')->setCode(function ($input, $output) {+ $helper = new QuestionHelper();+ $helper->ask($input, $output, new Question('Q1'));+ $helper->ask($input, $output, new Question('Q2'));+ $helper->ask($input, $output, new Question('Q3'));+ });++ $this->tester->setInputs(array('I1', 'I2', 'I3'));+ $this->tester->run(array('command' => 'foo'));++ $this->assertSame(0, $this->tester->getStatusCode());+ $this->assertEquals('Q1Q2Q3', $this->tester->getDisplay(true));+ }+ public function testGetStatusCode() { $this->assertSame(0, $this->tester->getStatusCode(), '->getStatusCode() returns the status code');
| * | ||
| * @return self | ||
| */ | ||
| publicfunctionsetInputs(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.
For this to work in ApplicationTester, the tester needs to set the input stream from this value.
SoCommandTester::createInputStream() should be shared between both testers (that's my motivation for a trait) andthis line should be added toApplicationTester::run().
| */ | ||
| publicfunctionsetInputs(array$inputs) | ||
| { | ||
| $this->inputs =$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.
the property does not exist in ApplicationTester, it should be created
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.
or moved to the trait along with:
/** @var StreamOutput */private$output;
Simperfit commentedNov 5, 2017
@chalasr So it seems that the test is failling. Is here something the modify directly in the Application to make this work properly ? |
370851e toefe61ccCompare| */ | ||
| trait TesterTrait | ||
| { | ||
| /** @var StreamOutput */ |
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.
not useful and differs withgetOutput() return type, I would drop it
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.
To me it is useful actually (for autocompletion and SCA). This trait needs aStreamOutput instance here, not anyOutputInterface. That's why I suggested this docblock in#24819 (comment) :)
| } | ||
| if ($this->inputs) { | ||
| $this->input->setStream(self::createStream($this->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.
Here is the failure culprit, I suggest toputenv('SHELL_INTERACTIVE=1') here and, afterrun(), either destroy it or restore its previous value if any
efe61cc to8b59c06CompareSimperfit commentedNov 5, 2017
Thanks for the review guys, tests are now green ;). |
| private$inputs =array(); | ||
| /** | ||
| * Gets the display returned by the last execution of the application. |
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.
command or application, same below
| return$this; | ||
| } | ||
| publicstaticfunctioncreateStream(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.
this one must stay private
| rewind($this->output->getStream()); | ||
| $display =stream_get_contents($this->output->getStream()); | ||
| putenv('SHELL_INTERACTIVE'); |
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.
putenv($shellInteractive ? "SHELL_INTERACTIVE=$shellInteractive" : 'SHELL_INTERACTIVE'); instead, removing the condition below
8b59c06 to3513236Comparechalasr left a comment
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.
👍
| publicfunctionrun(array$input,$options =array()) | ||
| { | ||
| $this->input =newArrayInput($input); | ||
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.
nitpicking but should be reverted ^^
3513236 toea86ed8Comparechalasr commentedNov 26, 2017
Thank you@Simperfit. |
… some code (Simperfit)This PR was merged into the 4.1-dev branch.Discussion----------[Console] add setInputs to ApplicationTester and share some code| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24784| License | MIT| Doc PR | todoI didn't implemented the tests because I don't know how to write them on ApplicationTester.Commits-------ea86ed8 [Console] add setInputs to ApplicationTest and share some code
I didn't implemented the tests because I don't know how to write them on ApplicationTester.