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.

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

Replace CLI testdox printer with rpkamp/fancy-testdox-printer #2920

Merged
merged 6 commits into from
Dec 15, 2017
Merged

Replace CLI testdox printer with rpkamp/fancy-testdox-printer #2920

merged 6 commits into from
Dec 15, 2017

Conversation

rpkamp
Copy link
Contributor

@rpkamp rpkamp commented Dec 14, 2017

Here is my initial implementation of replacing the current testdox printer with mine (rpkamp/fancy-testdox-printer).

A few remarks:

  • My printer is based on the PHPUnit\TextUI\ResultPrinter printer, and getting it to work with the PHPUnit\Util\TestDox\TextResultPrinter would be near impossible without a big overhaul of how printers are currently set up. So I put mine in the PHPUnit\TextUI namespace and load it as default when --testdox is provided on the command line. This does mean that:
    • People who have PHPUnit\Util\TestDox\TextResultPrinter set as their printer class will not see the new testdox printer after upgrading, but that may be fine.
    • Printing to file/html/xml remains unchanged
  • There is one failing test, the test with Balance cannot become negative and Balance cannot become negative 2 which should print only one line. I've tried several different solutions, but somehow I can't seem to get it to fit in the current design. Is this behaviour something you want to keep, or is it fine if it prints as two lines instead of one?

I do like the \PHPUnit\TextUI\TestDoxPrinterTest class, which is a class that tests a printer, with itself as input, to test that PHPUnit does it's work correctly, using PHPUnit. Very meta 😃

.travis.yml Outdated
@@ -32,7 +32,7 @@ before_script:
- echo 'assert.exception=On' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini

script:
- ./phpunit --coverage-clover=coverage.xml
- ./phpunit --coverage-clover=coverage.xml --testdox --color=auto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to demonstrate the output, can be removed later of course

Choose a reason for hiding this comment

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

Please do not change .travis.yml.

@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label Dec 15, 2017
@sebastianbergmann sebastianbergmann added this to the PHPUnit 7.0 milestone Dec 15, 2017
@sebastianbergmann
Copy link
Owner

There is one failing test, the test with Balance cannot become negative and Balance cannot become negative 2 which should print only one line. I've tried several different solutions, but somehow I can't seem to get it to fit in the current design. Is this behaviour something you want to keep, or is it fine if it prints as two lines instead of one?

I think it's not only fine but correct to print two lines instead of one. For consistency, though, the other TestDox outputs should be changed, too. Can you take care of that?

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Dec 15, 2017

My printer is based on the PHPUnit\TextUI\ResultPrinter printer, and getting it to work with the PHPUnit\Util\TestDox\TextResultPrinter would be near impossible without a big overhaul of how printers are currently set up. So I put mine in the PHPUnit\TextUI namespace and load it as default when --testdox is provided on the command line.

Makes sense. However, even though your printer is based on PHPUnit\TextUI\ResultPrinter it does not have to be under the PHPUnit\TextUI namespace / in the src/TextUI directory. Please move it to where the rest of the TestDox related code is. The class name should be PHPUnit\Util\TestDox\CliPrinter (unless you can think of a better name).

✔ Does something else with data set "one" [%f ms]
✔ Does something else with data set "two" [%f ms]


Choose a reason for hiding this comment

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

Any chance we can get rid of this extra blank line?

@sebastianbergmann
Copy link
Owner

Finally, the most important comment: thank you for taking the time to work on this!

/**
* @test
*/
public function it_should_print_the_class_name_of_test_class()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_the_class_name_of_test_class" is not in camel caps format

/**
* @test
*/
public function it_should_print_testdox_version_of_test_method()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_testdox_version_of_test_method" is not in camel caps format

/**
* @test
*/
public function it_should_print_check_mark_on_passed_test()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_check_mark_on_passed_test" is not in camel caps format

/**
* @test
*/
public function it_should_not_print_additional_information_on_test_passed()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_not_print_additional_information_on_test_passed" is not in camel caps format

/**
* @test
*/
public function it_should_print_cross_on_test_with_error()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_cross_on_test_with_error" is not in camel caps format

/**
* @test
*/
public function it_should_not_print_additional_information_on_risky_test_when_not_verbose()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_not_print_additional_information_on_risky_test_when_not_verbose" is not in camel caps format

/**
* @test
*/
public function it_should_print_additional_information_on_risky_test_when_verbose()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_additional_information_on_risky_test_when_verbose" is not in camel caps format

/**
* @test
*/
public function it_should_print_arrow_on_skipped_test()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_arrow_on_skipped_test" is not in camel caps format

/**
* @test
*/
public function it_should_not_print_additional_information_on_skipped_test_when_not_verbose()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_not_print_additional_information_on_skipped_test_when_not_verbose" is not in camel caps format

/**
* @test
*/
public function it_should_print_additional_information_on_skipped_test_when_verbose()

Choose a reason for hiding this comment

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

Method name "CliTestDoxPrinterTest::it_should_print_additional_information_on_skipped_test_when_verbose" is not in camel caps format

@codecov-io
Copy link

codecov-io commented Dec 15, 2017

Codecov Report

Merging #2920 into master will increase coverage by 0.06%.
The diff coverage is 88.65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2920      +/-   ##
============================================
+ Coverage      79.8%   79.87%   +0.06%     
- Complexity     2775     2822      +47     
============================================
  Files           105      107       +2     
  Lines          7313     7442     +129     
============================================
+ Hits           5836     5944     +108     
- Misses         1477     1498      +21
Impacted Files Coverage Δ Complexity Δ
src/Util/TestDox/TextResultPrinter.php 88.88% <ø> (ø) 4 <0> (ø) ⬇️
src/Util/TestDox/ResultPrinter.php 58.94% <100%> (-3.19%) 42 <0> (-3)
src/TextUI/Command.php 66.73% <100%> (-0.39%) 188 <0> (ø)
src/Util/TestDox/CliTestDoxPrinter.php 83.95% <83.95%> (ø) 27 <27> (?)
src/Util/TestDox/TestResult.php 94.64% <94.64%> (ø) 23 <23> (?)
src/Framework/TestCase.php 66.3% <0%> (-0.25%) 311% <0%> (ø)
src/TextUI/TestRunner.php 61.68% <0%> (-0.19%) 235% <0%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0b6081...cd956f7. Read the comment docs.

@sebastianbergmann
Copy link
Owner

Thank you, @rpkamp! I'll clean up the CS/WS issues.

@sebastianbergmann sebastianbergmann merged commit af9d6d0 into sebastianbergmann:master Dec 15, 2017
@rpkamp
Copy link
Contributor Author

rpkamp commented Dec 15, 2017

Cool, thanks! Any indication when this will be released? I'm guessing PHPUnit 7.0?

@sebastianbergmann
Copy link
Owner

Yes, this will be in PHPUnit 7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants