CI must dump diffs if strings do not match

[mvorisek]

Feature request

This is a feature request to improve CI ouput.

Currently "Failed asserting that two strings are identical." text is dumped when strings do not match.

I would expect full testual diff which is much more helpful to locate the issue.

I am not sure if this is related to Paratest switch, but phpunit on my local env. dumps the diffs as expected with default cli options.

[Wirone]

This is related to ParaUnit.

$ vendor/bin/phpunit tests/IntegrationTest.php --filter=tests/Fixtures/Integration/priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

Testing PhpCsFixer\Tests\IntegrationTest
F                                                                                                                                                                                                1 / 1 (100%)

Time: 00:00.154, Memory: 30.00 MB

There was 1 failure:

1) PhpCsFixer\Tests\IntegrationTest::testIntegration with data set "tests/Fixtures/Integration/priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test" (PhpCsFixer\Tests\Test\IntegrationCase Object (...))
Expected changes do not match result for "Integration of fixers: align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation." in "priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test".
Fixers applied:
align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 /**
  * foo
  *
- * bar2
+ * bar
  */
 '

vs.

$ vendor/bin/paraunit run tests/IntegrationTest.php --filter=tests/Fixtures/Integration/priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test

PARAUNIT v.1.3.0
by Francesco Panina, Alessandro Lai & Shark Dev Team @ Facile.it
F                                                                              1


Execution time -- 00:00:00

Executed: 1 test classes, 1 tests

Failures output:

1) PhpCsFixer\Tests\IntegrationTest::testIntegration with data set "tests/Fixtures/Integration/priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test" (PhpCsFixer\Tests\Test\IntegrationCase Object (...))
Expected changes do not match result for "Integration of fixers: align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation." in "priority/align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.test".
Fixers applied:
align_multiline_comment,phpdoc_trim_consecutive_blank_line_separation.
Failed asserting that two strings are identical.

1 files with FAILURES:
 PhpCsFixer\Tests\IntegrationTest

In short: this is out of the scope of this repo. On our side we only assert that 2 strings are equal. This leads to PHPUnit\Framework\ExpectationFailedException which contains SebastianBergmann\Comparator\ComparisonFailure which has getDiff() and prints the diff between actual and expected code.

So, diff is out of our control and problem lays between ParaUnit and PHPUnit. I can only guess it's related to stdout/stderr. @Jean85 is it possible to catch full PHPUnit's output? There's --stderr option that enforces output to be on stderr instead of stdout, would it be useful here?

[mvorisek]

please note stdout/stderr is not guaranteed to be synchronized together, thus the output must be dumped to one stream only

[mvorisek]

@Wirone what about using paratest which is 10x more popular than paraunit?

[Wirone]

@mvorisek please see this and this, basically there was a problem which I solved by changing tool rather than providing fix for previous one 🤷‍♂️😅. If we wanted to switch to ParaTest, we need to fix that test loader - switching to newer ParaTest version is not possible because of support for 7.4-8.0 in Fixer.

[mvorisek]

So the issues are for ParaTest < v7, ie. for PHP < 8.1. In order to fix issue, I propose to replace ParaUnit by ParaTest for PHP 8.1+.

[Wirone]

I don't know if ParaTest v7 handles it properly, did not test it as far as I remember. Using different tools depending on PHP version would complicate CI definition because job matrix would require more conditions, and composer.json would require on-the-fly changes (remove ParaUnit, install ParaTest). I don't know if it's something we want.. I understand that lack of diff is a disadvantage, but maybe let's try to contribute and fix it 🙂.

[keradus]

closing due to discussion

better to have blazing fast CI and ask developer, who is already working on PR and having it locally, to read fail diff on local machine,
than having CI taking 40mins

if ParaUnit will be implementing requested change, I'm glad to see it (even auto-enabled) for our usecase as well ;)

[mvorisek]

I am a contributor and this complicated my contributions. I am not saying to not use ParaUnit directly. This is a feature request to improve the test output. Until implemented - fixed to match phpunit informative value, I do not tjink it should be closed.

[Wirone]

@mvorisek I understand this is an impediment, but this is not something that we can fix on our side, so there's no point in keeping this open. I believe you should track facile-it/paraunit#215 and/or #6884 (because it would enable switching, or at least re-trying, ParaTest). Believe me, we're also affected by this, but as @keradus said it's small inconvenience comparing to the undisputable advantage: speed of the feedback 🙂.