Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Console] Cleaned up the unit tests. #6989

Merged
merged 1 commit into from Apr 9, 2013

Conversation

Projects
None yet
4 participants
Member

jakzal commented Feb 6, 2013

Cleaned up some unit tests in the Console component as suggested in #6935. I didn't fully cleanup the Application tests to not to delay this PR. I might do it later as a separate one.

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6935
License MIT
Doc PR n/a

@wouterj wouterj and 1 other commented on an outdated diff Feb 7, 2013

src/Symfony/Component/Console/Tests/ApplicationTest.php
@@ -145,7 +137,17 @@ public function testHasGet()
$p->setAccessible(true);
$p->setValue($application, true);
$command = $application->get('foo:bar');
- $this->assertEquals('Symfony\Component\Console\Command\HelpCommand', get_class($command), '->get() returns the help command if --help is provided as the input');
+ $this->assertInstanceOf('Symfony\Component\Console\Command\HelpCommand', $command, '->get() returns the help command if --help is provided as the input');
+ }
+
+ /**
+ * @expectedException \InvalidArgumentException
+ * @expectedExceptionMessage The command "foofoo" does not exist.
@wouterj

wouterj Feb 7, 2013

Member

Isn't it nicer to line these comments out, as we do with other PHPdoc annotations?

/**
 * @expectedException        \InvalidArgumentException
 * @expectedExceptionMessage The command "foofoo" does not exist.
 */
@jakzal

jakzal Feb 7, 2013

Member

Good point.

Owner

fabpot commented Feb 7, 2013

I'm against these changes because it's going to make merging between tests so much complicated.

Member

jakzal commented Feb 7, 2013

@fabpot that's true, I was actually thinking of this as well.

However, many of those tests are hard to follow. I could put additional effort to back-porting it to the other branches if needed. If that's not the way to go, please close this PR.

Owner

fabpot commented Feb 7, 2013

I'm not sure this is worth the effort. But if you want, doing that in the 2.1 branch would be, indeed, better (and cross your fingers for the merge into 2.2 ;)).

Contributor

vicb commented Feb 7, 2013

👍 tests should be considered as part of the documentation.

Member

wouterj commented Feb 7, 2013

👍 for the same reasons as @vicb I use tests a lot to discover how to use features that aren't documented yet in the documentation.

@fabpot fabpot added a commit that referenced this pull request Apr 9, 2013

@fabpot fabpot merged branch jakzal/console-tests-cleanup (PR #6989)
This PR was merged into the master branch.

Discussion
----------

[Console] Cleaned up the unit tests.

Cleaned up some unit tests in the Console component as suggested in #6935. I didn't fully cleanup the Application tests to not to delay this PR. I might do it later as a separate one.

| Q              | A                                        |
|--------------|--------------------------------|
| Bug fix?          | no |
|New feature? | no |
|BC breaks?    | no |
|Deprecations? |	no |
|Tests pass? | yes |
|Fixed tickets | #6935 |
|License | MIT |
|Doc PR | n/a |

Commits
-------

5ca04b0 [Console] Cleaned up the unit tests.
6f0a5ad

@fabpot fabpot merged commit 5ca04b0 into symfony:master Apr 9, 2013

1 check passed

default The Travis build passed
Details

@fabpot fabpot closed this Apr 9, 2013

@jakzal jakzal deleted the jakzal:console-tests-cleanup branch Apr 20, 2013

Member

jakzal commented Apr 20, 2013

@fabpot I forgot about this PR. Do you want me to submit another one for 2.1/2.2 branches?

Owner

fabpot commented Apr 21, 2013

@jakzal No, refactoring such as this one really belongs to master only. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment