Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Console][2.3] ApplicationTester - test stdout and stderr #17255

Closed
wants to merge 2 commits into from
Closed

[Console][2.3] ApplicationTester - test stdout and stderr #17255

wants to merge 2 commits into from

Conversation

SpacePossum
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Currently it is not possible to test application output of both stdout and stderr using the ApplicationTester. This makes it hard to check if an application writes to the correct output.

@SpacePossum SpacePossum changed the title [WIP][Console][2.3] ApplicationTester - test stdout and stderr [Console][2.3] ApplicationTester - test stdout and stderr Jan 4, 2016
@SpacePossum
Copy link
Contributor Author

Not sure how or if BC should be handled here, any opinions?

linaori
linaori reviewed Jan 4, 2016
View reviewed changes
src/Symfony/Component/Console/Output/ConsoleOutput.php
}

/**
* @return resource
*/
private function openOutputStream()
protected function openErrorStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion you shouldn't modify code just for tests. If this is something you'd like to mock or control in tests, you should inject an object that has the open methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a general rule, I agree. However ApplicationTester is not part of PHPUnit making mocking hard.
I don't see an option that will have less impact on the current code that will still allow for checking what is written to stdout and what to stderr.

For example the whole ConsoleOutput class could be mimic'ed and injected (some thing like https://github.com/symfony/symfony/pull/17255/files?diff=split#diff-3bb511669d5c28238608aa75cf865163L67), but would replace a part of the logic that should tested itself.

@SpacePossum SpacePossum mentioned this pull request Jan 5, 2016
Merged
@SpacePossum SpacePossum force-pushed the 2_3_feature_application_tester_diff_between_stdout_and_stderr branch from 007be88 to bbde83b Compare January 25, 2016 08:24
fabpot
fabpot reviewed Jan 25, 2016
View reviewed changes
src/Symfony/Component/Console/Tester/ApplicationTester.php
* * verbosity: Sets the output verbosity flag
* * interactive: Sets the input interactive flag
* * decorated: Sets the output decorated flag
* * verbosity: Sets the output verbosity flag
Copy link
Member

Choose a reason for hiding this comment

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

should be reverted

@SpacePossum SpacePossum changed the title [Console][2.3] ApplicationTester - test stdout and stderr [WIP][Console][2.3] ApplicationTester - test stdout and stderr Jan 25, 2016
@SpacePossum
Copy link
Contributor Author

I'll rework this and create a new proposal :)

@SpacePossum SpacePossum force-pushed the 2_3_feature_application_tester_diff_between_stdout_and_stderr branch 2 times, most recently from 1b66968 to f3fbc7a Compare January 27, 2016 14:24
@SpacePossum
Copy link
Contributor Author

@iltar please have a look at it again.
I've reverted all changes not needed, especially the visibility changes to the methods.
This PR uses reflection to gain access to the streams. It is now also free from BC breaks.

@SpacePossum SpacePossum changed the title [WIP][Console][2.3] ApplicationTester - test stdout and stderr [Console][2.3] ApplicationTester - test stdout and stderr Jan 27, 2016
@SpacePossum
Copy link
Contributor Author

Ping @nicolas-grekas, sorry for pinging you from a PR you are not involved in so far, hope it is OK.
I would like to move forward on a fix for this issue, one way or another. Please let me know if I'm on the right track. Than I know how I could make a temp. fix over at PHP-CS-Fixer (PHP-CS-Fixer/PHP-CS-Fixer#1654) while this PR @ SF is being processed.

@SpacePossum
Copy link
Contributor Author

ping @stof (sorry, but please have look)
suggestions welcome :)

@keradus
Copy link
Member

keradus commented Feb 25, 2016

Any news on that?

fabpot
fabpot reviewed Feb 29, 2016
View reviewed changes
src/Symfony/Component/Console/Tester/ApplicationTester.php
@@ -108,7 +163,7 @@ public function getInput()
/**
* Gets the output instance used by the last execution of the application.
*
* @return OutputInterface The current output instance
* @return OutputInterface|ConsoleOutputInterface The current output instance
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted as ConsoleOutputInterface extends OutputInterface

@SpacePossum SpacePossum force-pushed the 2_3_feature_application_tester_diff_between_stdout_and_stderr branch 2 times, most recently from 4e1c9c3 to 3b75524 Compare March 9, 2016 12:35
@SpacePossum SpacePossum force-pushed the 2_3_feature_application_tester_diff_between_stdout_and_stderr branch from 5af06ad to a5d522a Compare March 9, 2016 12:37
@SpacePossum
Copy link
Contributor Author

Reworked, happy to take notes/comments

fabpot
fabpot reviewed Mar 10, 2016
View reviewed changes
src/Symfony/Component/Console/Tester/ApplicationTester.php
*/
private $output;
private $captureOutputStreamsIndependent = false;
Copy link
Member

Choose a reason for hiding this comment

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

$captureStreamsIndependently?

@fabpot
Copy link
Member

fabpot commented Mar 10, 2016

Thank you @SpacePossum.

@fabpot fabpot closed this in af36c83 Mar 10, 2016
@SpacePossum SpacePossum deleted the 2_3_feature_application_tester_diff_between_stdout_and_stderr branch March 10, 2016 21:29
@SpacePossum
Copy link
Contributor Author

Thank you all for reviewing! :)

keradus added a commit to PHP-CS-Fixer/PHP-CS-Fixer that referenced this pull request Apr 15, 2016
@keradus
minor #1654 Split output to stderr and stdout (SpacePossum)
514410c 
This PR was merged into the 1.12 branch.

Discussion
----------

Split output to stderr and stdout

For more correct testing I need symfony/symfony#17255 to be resolved.

issue: #1579 (and others)

Commits
-------

880f58d Split output to stderr and stdout.
@fabpot fabpot mentioned this pull request May 13, 2016
Merged
@SpacePossum SpacePossum mentioned this pull request Apr 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Console Feature Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@SpacePossum @keradus @fabpot @linaori @javiereguiluz @nicolas-grekas @carsonbot