Allow to define options to display colors #1527
Conversation
|
Doesn't this break BC by enforcing that --colors must now have an argument? |
|
Exactly the opposite, it doesn't. |
| { | ||
| $currentValue = strtolower($value); | ||
|
|
||
| if (in_array($currentValue, array('auto', 'tty', 'if-tty'))) { |
There was a problem hiding this comment.
I think 'true' missing here ?
|
I will close that pull request in favor of #1526, I don't @sebastianbergmann is going to have a different opinion of you guys. |
|
I decided to change some stuff, create a PHPT test and keep the pull request opened for a while. |
Allow to define options to display colors
|
:-( |
|
@remicollet What's wrong? |
|
The old colors=true in configuration file make terribly ugly output in build log (reason for the change to detect if colors are possible) Allowing colors=always, which is going to be used by some projects (to get colors in travis) will put us in the same bad situation as previous, having to patch tons of configuration files. More, it will make the configuration file (which use colors=always) incompatible with older phpunit version, thus, raising dependency on phpunit 4.x only for this "cosmetic" feature I still really think that change need to be applied in hasColorSupport() to properly detect case where colors are possible, and that, if this "force" mode is allowed, it should be only allowed from command line, not from configuration file. |
|
I understand your perspective, Remi, but I think for PHPUnit we should focus on its users and not on the users of its users, if that makes sense. Patches to improve |
|
@sebastianbergmann thanks |
|
@remicollet, I saw what you did on #1529, but as you already know what you tried to fix in #1458 was not fixed yet and it must be fixed on Environment. Now you can use There is quite difference between |
|
@henriquemoody sorry, I haven't understood your last comment :( My feeling:
I don't say new feature are not allowed in configuration. I just mean that some project are going to use colors="always" and raise dependency on phpunit 4.6 just for this "comestic", which is terrible tricky. I think hasColorSupport() is enough for me. If people want to improve it, they can (ex add detection for Travis), but I really don't care:
Remember: PHPUnit is a tool to run unit test. I really think we should focus on test feature. |
#1458 may fix the problem of having
As I can see, looks like you have cases when is not supported but it still is enabled.
No, it's not, it's a new feature. I'm sure you know BTW, hasColorSupport() as you can see in its docblock it is just a copy of Symfony\Console and I haven't seem people reporting bugs like that, maybe there is something I didn't catch yet.
Agreed, but it doesn't mean we don't have to care about anything else, I think that's the same reason you sent #1458. |
fix ci errors by following change of phpunit.xml's attribute usage. * Deleted `colors="auto"`. currently, `colors` value must be `true` or `false`, and default is `auto`. (and all current supported versions are default `auto`) see also: sebastianbergmann/phpunit#1527 * Move whitelist setting attributes into `whitelist` tag attributes targets: - `addUncoveredFilesFromWhitelist` - `processUncoveredFilesFromWhitelist` those two kind of attributes have deleted from `phpunit` tag. * Deleted `logIncompleteSkipped` attribute `logIncompleteSkipped` is no longer allowed in phpunit.xml. and because, we have no `skipped` test, we don't need it. so, simply I delete it.
fix ci errors by following change of phpunit.xml's attribute usage. * Deleted `colors="auto"`. currently, `colors` value must be `true` or `false`, and default is `auto`. (and all current supported versions are default `auto`) see also: sebastianbergmann/phpunit#1527 * Move whitelist setting attributes into `whitelist` tag attributes targets: - `addUncoveredFilesFromWhitelist` - `processUncoveredFilesFromWhitelist` those two kind of attributes have deleted from `phpunit` tag. * Deleted `logIncompleteSkipped` attribute `logIncompleteSkipped` is no longer allowed in phpunit.xml. and because, we have no `skipped` test, we don't need it. so, simply I delete it. * Add testsuite `name` In latest version, `name` attribute is must be set.
Related to #1458 and #1525.
It's basically the same idea of #1526 but this also allows you to use the configuration file to define the colors while #1526 only allows you to do it by command line.