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 JUnit missing PHPT results #3437

Merged
merged 6 commits into from Dec 6, 2018
Merged

Fix JUnit missing PHPT results #3437

merged 6 commits into from Dec 6, 2018

Conversation

epdenouden
Copy link
Contributor

@epdenouden epdenouden commented Dec 5, 2018

Fixes #3436

image

Fixed JUnit logger by implementing more methods from TestCase in PhptTestCase.

Changes:

  • JUnit logger no longer checks for strict instanceof TestCase
  • PhptTestCase always returns number of run assertions of 1
  • PhptTestCase now correctly replies it doesn't use a dataprovider
  • PhptTestCase now returns the STDOUT of a test for <system-out> element
  • add tests for PHPT results being included in TeamCity and JUnit logs

Thanks to @Naktibalda for noticing the bug.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #3437 into 7.5 will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                7.5    #3437      +/-   ##
============================================
+ Coverage     83.31%   83.41%   +0.09%     
- Complexity     3612     3614       +2     
============================================
  Files           143      143              
  Lines          9619     9627       +8     
============================================
+ Hits           8014     8030      +16     
+ Misses         1605     1597       -8
Impacted Files Coverage Δ Complexity Δ
src/Util/Log/JUnit.php 94.11% <ø> (+4.11%) 30 <0> (-2) ⬇️
src/Runner/PhptTestCase.php 80.17% <100%> (+1.1%) 83 <4> (+4) ⬆️
src/Util/Log/TeamCity.php 60.68% <0%> (+1.37%) 54% <0%> (ø) ⬇️

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 543f76a...e6ab8c4. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #3437 into 7.5 will decrease coverage by 0.04%.
The diff coverage is 52.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##                7.5    #3437      +/-   ##
============================================
- Coverage     83.31%   83.26%   -0.05%     
- Complexity     3612     3614       +2     
============================================
  Files           143      143              
  Lines          9619     9627       +8     
============================================
+ Hits           8014     8016       +2     
- Misses         1605     1611       +6
Impacted Files Coverage Δ Complexity Δ
src/Util/Log/JUnit.php 91.17% <ø> (+1.17%) 30 <0> (-2) ⬇️
src/Runner/PhptTestCase.php 76.65% <52.94%> (-2.42%) 83 <4> (+4)

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 543f76a...b973fe7. Read the comment docs.

@epdenouden epdenouden changed the title Implement more TestCase methods so JUnit logger can handle PHPT Fix JUnit missing PHPT results Dec 5, 2018
@sebastianbergmann sebastianbergmann merged commit 51a7c5b into sebastianbergmann:7.5 Dec 6, 2018
Copy link
Contributor

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

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

Sorry for late response.
Comment below.

@@ -318,10 +313,6 @@ public function startTest(Test $test): void
*/
public function endTest(Test $test, float $time): void
{
if (!$test instanceof TestCase) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change fixes phpttestcase, but it will cause fatal errors for third party implementations of Test interface having no getNumAssertions or hasOutput methods.
It would be the best to check if those methods exist before calling them.
Up until 7.3.5 here was a condition to check if hasOutput method exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, now I understand the history here a bit better. Other implementations can run into the exact same problem of course.

I will add some safety around this.

@epdenouden epdenouden deleted the issue-3436-junit-logger-ignores-phpt-tests branch December 6, 2018 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants