Cache on successful file processing

[yguedidi]

New version of #3604
Closes rectorphp/rector#7770
Now a file will be cached only when not in dry run mode

needs first:

• Add e2e tests for dry run #3616
• Add e2e tests for consecutive changing runs #3666

[yguedidi]

here the added condition

[yguedidi]

here the added condition

[samsonasik]

I think this one need e2e test to avoid regression and back and forth check, eg:

• multiple --dry-run check should ok.
• multiple succesfull keep changing, eg, the following code got twice changed should ok.

if ($a && $b) {
    return true;
}

on config:

<?php

declare(strict_types=1);

use Rector\CodingStyle\Rector\Stmt\NewlineAfterStatementRector;
use Rector\Config\RectorConfig;
use Rector\EarlyReturn\Rector\If_\ChangeAndIfToEarlyReturnRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->rule(ChangeAndIfToEarlyReturnRector::class);
    $rectorConfig->rule(NewlineAfterStatementRector::class);
};

First: https://getrector.com/demo/55b308a8-af17-48cb-b2be-9c0d26ab60c4
Second: https://getrector.com/demo/53cd71fb-1851-44d0-aa1e-ba7e5329db77

[yguedidi]

Thanks for guidance! Will work on separate PRs this week-end to add those e2e tests

[staabm]

Is this still WIP ?

[samsonasik]

It needs one more e2e test for consecutive succesfull.

for consecutive dry-run, there was already PR #3616

[yguedidi]

I planned to work on that e2e test this week-end

[yguedidi]

@samsonasik @staabm here it is: #3666

[staabm]

why are these test-changes required?

existing tests shouldn't be changed with new PRs

[yguedidi]

because previous logic is splitted in 2 methods now, I renamed one so maybe it's clearer

[staabm]

I think the PR itself needs a new test-case reproducing the issue it is trying to fix

[yguedidi]

@staabm good point! I'll try to come with a test in following days, but the final code is ready if you want to have a look, see that all tests pass now, even the consecutive runs test.
@samsonasik Idea is to cache files only when Rector didn't changed them, so when they are in their final state. So following runs will still go through previously changed files to attempt to change them again.
Maybe not the best way, but at least it preserve actual Rector vision on multiple runs and makes the cache reliable

Credits to @stof for the idea! Thank you

[samsonasik]

Please rename commit message with double quote "" , that will cause failure build downgrade

[yguedidi]

@samsonasik that revert commit is a temporary one just to generate a failing build, to show the issue.
will remove it from the final PR, showing that the PR solves the issue

[yguedidi]

Here the failing test without the fix: https://github.com/rectorphp/rector-src/actions/runs/4850942011/jobs/8644319975?pr=3614
See that with the fix by caching at the end instead of caching at the beginning, that previously failing test is now green: https://github.com/rectorphp/rector-src/actions/runs/4850961546/jobs/8644357227?pr=3614

[yguedidi]

we expect the timeout error to stay even with cache, and now the timouted files are not cached because we cache on success instead of caching at the beginning of the processing

[yguedidi]

used 1 second timeout to maximize chances of timeout

[yguedidi]

used a big rule set to maximize chances of timeout

[yguedidi]

used a big file from src to maximize chances of timeout

[yguedidi]

@TomasVotruba @samsonasik @staabm this is now ready for review!

[samsonasik]

Looking good for me 👍

[TomasVotruba]

Thank you 👍

[staabm]

I think we need the same fix in https://github.com/easy-coding-standard/easy-coding-standard. @yguedidi could you check that?

[yguedidi]

@staabm I'm not using ECS, so I can't guarantee that I can come with a PR any time soon there sorry.
It's fine if some else inspire from this PR to implement it in ECS :)