Fixes showing files changed in JSON formatter

[peterfox]

Changes

• Changes how the JSON formatter works in getting the diffs from the process results.
  • Still ignores diffs that are an empty string when not using no-diffs
  • Hides diffs if using no-diffs
• Adds a test for the JSON Formatter for this with a JSON fixture file

Why

Currently running ./vendor/bin/rector --output-format=json --no-diffs you will only get the count of how many files changed. This change will show the list as well.

{
    "totals": {
        "changed_files": 1,
        "errors": 0
    },
    "changed_files": [
        "some/file.php",
    ],
}

See discussion in #7807

Notes

Rushed this one together, if I can improve the test or the way we determine to show the diff when it's an empty string, feedback would be greatly appreciated.

[TomasVotruba]

This seems a bit hacky and error prone. Any chance to test output directly without it?

[samsonasik]

The output formatter is echo directly, so the only way to get output is via buffer

rector-src/src/ChangesReporting/Output/JsonOutputFormatter.php

Line 61 in 540fbfa

echo $json . PHP_EOL;

and the content read will be mixed on phpunit side expected output check.

The other way is by use \n over PHP_EOL on echo, and normalize the rest.

[samsonasik]

Okay, I got it, removed the ob_start() hack and fix with trim and re-add PHP_EOL during parent::expectOutputString($expectedOutput) 👍 e1d3d99

[TomasVotruba]

Maybe there could a factory service that creates and returns string and that tested with assertSame()

[samsonasik]

The parent::expectOutputString() is simpler imo, see implementation on last commit e1d3d99

no more ob_start() thing as handled in phpunit itself that handle the buffer output internally.

Just added the tweak do to the PHP_EOL enforcement on the echo, so that needs the trim and re-add the PHP_EOL

[TomasVotruba]

Overriding protected method seems extra complexity I struggle to read.
I still prefer simple return string

[samsonasik]

the method name is different: expectOsOutputString vs expectOutputString

should be private instead :), will update and clear the logic :)

[samsonasik]

I've clean up unnecessary new method and just directly normalize it on $this->expectOutputString() 9cc1408 👍

should be ok now 👍

[samsonasik]

@TomasVotruba I created alternative PR for use of JsonOutputFactory for testing:

• [ChangesReporting][Alternative] Collect changed_files on --no-diffs with json output with use of JsonOutputFactory #7821

[TomasVotruba]

This turned into a bit of mess. @peterfox Can you force-push your last changes? I'll look into it to get this finished .)

[peterfox]

@TomasVotruba I've rebased and force pushed the original code

[TomasVotruba]

Thank you 👍 I'll look into it and finish it

[samsonasik]

@TomasVotruba just for note, this also may need check on ApplicationFileProcessor for cli command to append the empty FileDiff with add list file path, to avoid still not listed with --no-diffs:

➜  rector-src git:(use-factory) ✗ bin/rector process src/Application/VersionResolver.php  --output-format=json --no-diffs  --dry-run
{
    "totals": {
        "changed_files": 1,
        "errors": 0
    }
}

see my note at separate PR

#7821 (comment)