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

Catch and display errors during bootstrapping when using testdox printer #3108

Closed
wants to merge 1 commit into from
Closed

Conversation

rpkamp
Copy link
Contributor

@rpkamp rpkamp commented Apr 28, 2018

Fixes #3107

It changes quite some stuff around, not sure if you're fine with breaking BC for this. If not, we'd need to find a different solution.

Example output:

screenshot from 2018-04-28 17-52-54

@sebastianbergmann
Copy link
Owner

Thank you for working on this!

Can you elaborate on the BC break?

The tests fail on PHP 7.3.

@sebastianbergmann
Copy link
Owner

@mhujer PHPStan complains about a function that does not exist. That is, of course, correct but not what we want here.

@mhujer
Copy link
Contributor

mhujer commented Apr 29, 2018

@sebastianbergmann It should be enough to add it to ignoreErrors section in phpstan-tests.neon:

        # intentionally non existent function in tests/Regression/GitHub/3107/Issue3107Test.php
        - '#Function does_not_exist not found#'

(should be copy&paste-able)

@rpkamp
Copy link
Contributor Author

rpkamp commented Apr 30, 2018

Checking again I don't think there is a serious BC break after all, since the class that changed the most (now PHPUnit\Util\TestDox\RunningTestResult, used to be PHPUnit\Util\TestDox\TestResult - which is now an interface) was final anyway. so no-one could have extended that. They might have been using it, but that seems like a stretch.

@codecov-io
Copy link

codecov-io commented Apr 30, 2018

Codecov Report

Merging #3108 into master will increase coverage by 0.11%.
The diff coverage is 95.69%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3108      +/-   ##
============================================
+ Coverage     80.91%   81.03%   +0.11%     
- Complexity     2903     2912       +9     
============================================
  Files           108      109       +1     
  Lines          7598     7628      +30     
============================================
+ Hits           6148     6181      +33     
+ Misses         1450     1447       -3
Impacted Files Coverage Δ Complexity Δ
src/Util/TestDox/CliTestDoxPrinter.php 88.09% <100%> (+4.14%) 28 <0> (+1) ⬆️
src/Util/TestDox/RunningTestResult.php 94.64% <94.64%> (ø) 24 <24> (?)
src/Util/TestDox/BootstrappingTestResult.php 96.29% <96.29%> (ø) 8 <8> (?)
src/Framework/TestResult.php 72.09% <0%> (+0.25%) 158% <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 85efea3...1d4d0e0. Read the comment docs.

@rpkamp
Copy link
Contributor Author

rpkamp commented Apr 30, 2018

  • Fixed PHPStan error by adding it to ignoreErrors - thanks @mhujer
  • Fixed tests in PHP 7.3 by looking for %Sdoes_not_exist() in EXPECTF test
  • Fixed CS errors

Commit amended and force pushed, all tests are green now

ping @sebastianbergmann

@sebastianbergmann
Copy link
Owner

Merged manually, thanks.

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.

4 participants