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

Fix testdox printer #5521

Closed
wants to merge 3 commits into from
Closed

Fix testdox printer #5521

wants to merge 3 commits into from

Conversation

ghostwriter
Copy link
Contributor

@ghostwriter ghostwriter commented Sep 21, 2023

Following #5488 & #5518

@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner labels Sep 22, 2023
@sebastianbergmann sebastianbergmann added this to the PHPUnit 10.4 milestone Sep 22, 2023
@sebastianbergmann
Copy link
Owner

Thank you for working on this. I will look at this as soon as I find the time and provide more detailed feedback.

I am sorry that my initial feedback, which is based on just looking at the commit messages, might sound more negative than I actually mean it.

I do not like commit messages like "Update ". Please squash your commits into a single commit with a commit message such as "Closes #5488 and #5518".

@localheinz
Copy link
Collaborator

@sebastianbergmann

Restarted jobs following shivammathur/setup-php#769 (comment).

Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #5521 (451adb1) into main (95e7930) will decrease coverage by 0.11%.
The diff coverage is 75.80%.

@@             Coverage Diff              @@
##               main    #5521      +/-   ##
============================================
- Coverage     88.06%   87.95%   -0.11%     
- Complexity     6318     6363      +45     
============================================
  Files           675      692      +17     
  Lines         20248    20406     +158     
============================================
+ Hits          17831    17948     +117     
- Misses         2417     2458      +41     
Files Coverage Δ
...stMethod/Subscriber/ExecutionStartedSubscriber.php 100.00% <100.00%> (ø)
...riber/TestRunnerTriggeredDeprecationSubscriber.php 100.00% <100.00%> (ø)
...tMethod/Subscriber/TestSuiteFinishedSubscriber.php 100.00% <100.00%> (ø)
...stMethod/Subscriber/TestSuiteStartedSubscriber.php 100.00% <100.00%> (ø)
.../Subscriber/TestTriggeredDeprecationSubscriber.php 100.00% <100.00%> (ø)
...ethod/Subscriber/TestTriggeredNoticeSubscriber.php 100.00% <100.00%> (ø)
...thod/Subscriber/TestTriggeredWarningSubscriber.php 100.00% <100.00%> (ø)
...scriber/BeforeTestClassMethodErroredSubscriber.php 0.00% <0.00%> (ø)
...ubscriber/TestRunnerTriggeredWarningSubscriber.php 0.00% <0.00%> (ø)
...stMethod/Subscriber/TestSuiteSkippedSubscriber.php 0.00% <0.00%> (ø)
... and 9 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sebastianbergmann
Copy link
Owner

I am sorry, but I am not able to review this in time for PHPUnit 10.4.

@sebastianbergmann
Copy link
Owner

I would like to start with an apology: I am sorry that I was not able to review this pull request sooner. I am also sorry that I have to tell you that I cannot accept this pull request.

Consider this test case class:

Test.php

<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

final class Test extends TestCase
{
    public function testNoIssues(): void
    {
        $this->assertTrue(true);
    }

    public function testFailure(): void
    {
        $this->assertTrue(false);
    }

    public function testDeprecation(): void
    {
        trigger_error('deprecation', E_USER_DEPRECATED);

        $this->assertTrue(true);
    }

    public function testNotice(): void
    {
        trigger_error('notice', E_USER_NOTICE);

        $this->assertTrue(true);
    }

    public function testRisky(): void
    {
    }

    public function testWarning(): void
    {
        trigger_error('warning', E_USER_WARNING);

        $this->assertTrue(true);
    }
}

By default, the default result printer produces this output:

$ phpunit Test.php
PHPUnit 11.0-g451adb1f6d by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)

Time: 00:00.018, Memory: 4.00 MB

There was 1 failure:

1) Test::testFailure
Failed asserting that false is true.

/home/sb/Test.php:13

--

There was 1 risky test:

1) Test::testRisky
This test did not perform any assertions

/home/sb/Test.php:30

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

And here is the output produced by the default result printer when asked for more details:

$ phpunit --display-deprecations --display-notices --display-warnings Test.php
PHPUnit 11.0-g451adb1f6d by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)

Time: 00:00.018, Memory: 4.00 MB

There was 1 failure:

1) Test::testFailure
Failed asserting that false is true.

/home/sb/Test.php:13

--

There was 1 risky test:

1) Test::testRisky
This test did not perform any assertions

/home/sb/Test.php:30

--

1 test triggered 1 warning:

1) /home/sb/Test.php:36
warning

Triggered by:

* Test::testWarning
  /home/sb/Test.php:34

--

1 test triggered 1 notice:

1) /home/sb/Test.php:25
notice

Triggered by:

* Test::testNotice
  /home/sb/Test.php:23

--

1 test triggered 1 deprecation:

1) /home/sb/Test.php:18
deprecation

Triggered by:

* Test::testDeprecation
  /home/sb/Test.php:16

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

Without your proposed changes, the TestDox result printer produces this output:

$ phpunit --display-deprecations --display-notices --display-warnings --testdox Test.php
PHPUnit 11.0-g41c46329d4 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)

Time: 00:00.017, Memory: 4.00 MB

Unnamed Tests
 ✔ No issues
 ✘ Failure
   │
   │ Failed asserting that false is true.
   │
   │ /home/sb/Test.php:13
   │
 ✔ Deprecation
 ✔ Notice
 ☢ Risky
 ✔ Warning

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

With your proposed changes, the TestDox result printer produces this output:

$ phpunit --display-deprecations --display-notices --display-warnings --testdox Test.php
PHPUnit 11.0-g451adb1f6d by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)

Time: 00:00.017, Memory: 4.00 MB

Unnamed Tests
 ✔ No issues
 ✘ Failure
   │
   │ Failed asserting that false is true.
   │
   │ /home/sb/Test.php:13
   │
 ? Deprecation
 ? Notice
 ☢ Risky
 ⚠ Warning

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

I am sorry to say this, but this is not really an improvement: the details for deprecations, notices, risky considerations, and warnings are still missing.

Where I do see a significant change is when it comes to the plain-text TestDox logger. Here is its output without your changes:

$ phpunit --testdox-text php://stdout Test.php                                      
PHPUnit 11.0-g41c46329d4 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)Unnamed Tests
 [x] No issues
 [ ] Failure
 [x] Deprecation
 [x] Notice
 [ ] Risky
 [x] Warning



Time: 00:00.018, Memory: 4.00 MB

There was 1 failure:

1) Test::testFailure
Failed asserting that false is true.

/home/sb/Test.php:13

--

There was 1 risky test:

1) Test::testRisky
This test did not perform any assertions

/home/sb/Test.php:30

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

And here is its output with your changes:

PHPUnit 11.0-g451adb1f6d by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.1

.FDNRW                                                              6 / 6 (100%)Unnamed Tests
 [x] No issues
 [ ] Failure
      ┐
      ├ Failed asserting that false is true.
      │
      │ /home/sb/Test.php:11
      ┴
 [ ] Deprecation
      ┐
      ├ deprecation
      │
      │ /home/sb/Test.php:16
      ┴
 [ ] Notice
      ┐
      ├ notice
      │
      │ /home/sb/Test.php:23
      ┴
 [ ] Risky
      ┐
      ├ This test did not perform any assertions
      │
      │ /home/sb/Test.php:30
      ┴
 [ ] Warning
      ┐
      ├ warning
      │
      │ /home/sb/Test.php:34
      ┴



Time: 00:00.017, Memory: 4.00 MB

There was 1 failure:

1) Test::testFailure
Failed asserting that false is true.

/home/sb/Test.php:13

--

There was 1 risky test:

1) Test::testRisky
This test did not perform any assertions

/home/sb/Test.php:30

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Warnings: 1, Deprecations: 1, Notices: 1, Risky: 1.

However, I am not sure whether this can still be considered plain-text now that characters such as , , etc. are used. Furthermore, but that is off-topic, I am beginning to think that the plain-text TestDox logger should be discontinued.

I have outlined my plan for displaying test issues when --testdox is used here: #5518 (comment)

I hope that you understand why I have decided to not merge your proposed changes. I appreciate the time you invested in preparing this pull request and thank you for your contribution.

@ghostwriter ghostwriter deleted the bugfix/testdox-printer branch January 7, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants