sebastianbergmann / phpunit

The PHP Unit Testing framework.
https://phpunit.de/
BSD 3-Clause "New" or "Revised" License
19.68k stars 2.2k forks source link

Text coverage report no longer shows colours #5881

Open Bilge opened 3 months ago

Bilge commented 3 months ago
Q A
PHPUnit version 10.5.24
PHP version 8.2.15
Installation Method Composer

Summary

PHPUnit 9 --coverage-text with colors="true" would output the report in colour, making it easy to identify which files were uncovered. PHPUnit 10 does not do this.

Current behavior

Colours are gone 😞

image

How to reproduce

phpunit --coverage-text

Expected behavior

Colours.

image

sebastianbergmann commented 3 months ago

I cannot reproduce that using PHPUnit's own test suite on branch 10.5:

grafik

sebastianbergmann commented 3 months ago

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

Bilge commented 3 months ago

I think it is less about the test case and more about the environment. I was able to get it to work in WSL 2 (Ubuntu), but it does not work in GitHub Actions and it does not work in Git Bash (mintty) on Windows.

Perhaps you are doing some (invalid) terminal capability detection that concludes these environments do not support colours even though they do. Again, this worked in PHPUnit 9 and earlier; it is only broken in PHPUnit 10.

sebastianbergmann commented 3 months ago

I am sorry to say this, but this report is not actionable for me. I cannot and will not work on this myself, as I do not know how. I somebody, maybe you, finds the cause for this issue in their environment and sends a pull request then I will, of course, consider that.

Bilge commented 3 months ago

Cannot = false. Will not = noted. You have access to GitHub Actions, same as me. Nevertheless, I'll try to get to the bottom of it myself. Alone.

sebastianbergmann commented 3 months ago

Terminal capabilities are checked by sebastian/environment. PHPUnit 9 uses ^5 of that library, PHPUnit 10 uses ^6.

Looking at https://github.com/sebastianbergmann/environment/compare/5.1.5...6.1.0#files_bucket, I do not see any code changes between the version used by PHPUnit 9 and PHPUnit 10 that are related to detecting terminal capabilities. https://github.com/sebastianbergmann/environment/blob/5.1.5/src/Console.php#L55-L77 and https://github.com/sebastianbergmann/environment/blob/6.1.0/src/Console.php#L55-L77 are identical, AFAICS.

Bilge commented 3 months ago

Setting TERM=xterm fixes it for mintty (it was previously xterm-256color). Still no idea what changed to break GitHub Actions.

sebastianbergmann commented 3 months ago

Looks like the original code in Symfony Console that I copied/adapted has changed. It now looks like so.

However, that does not explain the difference between PHPUnit 9 and PHPUnit 10 in your environment.

Bilge commented 3 months ago

At some point (somewhat recently) I manually changed the TERM setting from xterm to xterm-256color, so that is resolved. It is just GitHub Actions that remains unresolved.


EDIT: No, it doesn't (what am I saying?) The fact of the matter is, xterm-256color was (and is) working perfectly fine in PHPUnit 9, for some reason.

Bilge commented 3 months ago

I think a valid question would be, why does PHPUnit even attempt to do capabilities detection when colors=true? If we're explicitly enabling it, the software should take that as read. I would expect the current behaviour only when colors=auto. Compare to ls:

   --color[=WHEN]
          colorize the output; WHEN can be 'always' (default if omitted), 'auto', or 'never'; more info below

Further, the whole notion of capabilities detection seems somewhat of a nonsense because all the escapes are hardcoded anyway; it's not like we're pulling the correct escapes from a term capabilities database (as should be done if we're being strictly correct).

GitHub Actions environment is a tricky case because it does not behave like a valid terminal environment supporting colour, yet it does support it anyway. Correct colour capabilities detection (e.g. using tput) generally fails there. Of course, it would still be nice to get to the bottom of what changed between 9 and 10.

Bilge commented 3 months ago

Although I haven't been able to step debug/trace it, it seems clear from this behaviour that in PHPUnit 9, whether due to a bug or otherwise, capabilities simply weren't being checked. In PHPUnit 10, whether internally or otherwise, capabilities are now being consulted before outputting colours, as we can see by colours now being disabled when the (limited) environment detection kicks in for unsupported environments. Therefore, looking for changes in the code for capabilities support is a red herring, because in the previous version, capabilities support wasn't even being consulted at all.

I still think the previous behaviour was correct. If we want to do environment detection, a new colors=auto mode would be preferable (and update the old detection code copied from Symfony). This is backwards-compatible behaviour then.

Bilge commented 3 months ago

To get colour support back in all environments, (i.e. the old PHPUnit 9 behaviour) was must set TERM_PROGRAM=Hyper in the environment. e.g.

    "scripts": {
         "test": ["@putenv TERM_PROGRAM=Hyper", "phpunit"],
    }

This covers off Windows and GitHub Actions environments equally.

Bilge commented 2 months ago

Looks like the original code in Symfony Console that I copied/adapted has changed. It now looks like so.

What are we going to do about this? Just copy/paste the updated code, or try to convince them to make this code public?

llaville commented 1 month ago

@Bilge Thanks for your solution, it helps me to re-introduced PHPUnit code coverage feature that I've not used since a long time.

And it works well on GitHub Actions and WSL 2 (for my locally tests) on PHPUnit 10.5 !

TL;DR but more details (with screenshots) are given at https://github.com/llaville/box-manifest/discussions/12