Add e2e tests for consecutive changing runs #3666
Conversation
yguedidi
force-pushed
the
e2e-consecutive
branch
from
7fcc990
to
da611a0
Compare
| // so we can use helper classes here | ||
| require_once __DIR__ . '/../vendor/autoload.php'; | ||
|
|
||
| $e2eCommand = 'php '. $rectorBin .' process --no-ansi -a '. $autoloadFile; |
There was a problem hiding this comment.
this new runner removed the --dry-run option
| if (isset($argv[1]) && $argv[1] === '-o') { | ||
| $expectedDiff = $argv[2]; | ||
| } else { | ||
| $expectedDiff = 'expected-output.diff'; | ||
| } |
There was a problem hiding this comment.
and adds support for a specific expected output file
yguedidi
force-pushed
the
e2e-consecutive
branch
from
da611a0
to
99f2408
Compare
|
@samsonasik fixed |
|
@samsonasik Seeing that this PR make #3614 fails, I get a bit confused.. But I'm wondering if it's really a use case we want that users have to run Rector multiple times to get the final fixed version of a fila? @TomasVotruba @samsonasik what do you think? |
|
@samsonasik made it a draft PR meanwhile we discuss the above point |
|
Consecutive run can happen, when changed node just reprinted, and require the print to build the new type, and then, next run of Rector required to finalize it, in rector-src, that happen by rectify auto commit CI until no longer change. |
|
I understand it can happen, but I believe from a user perspective it's not something expected. |
|
Rector auto refresh type and continue to next rule by default, but on some cases, eg: add new line and spacing, that need to be re-read after. That known limitation https://github.com/rectorphp/rector#known-drawbacks Trust me, that expected to have multiple run. |
|
I do trust you, I'm just wondering why can't we make that "second run" programatically in some optimized way, so it's not up to end users, as I think they will expect Rector to do its full job by one run. |
|
Maybe you know why some code styling rectors were introduced in the first place? |
|
That's complex, it is not only that, that just an example, other factor can happen, eg: on multiple re-print consecutively , rector expected to stop, otherwise, that will be infinite loop. |
is this covered by I test? I may try something :) |
|
Mostly on multiple rules usage, you can see tests/Issues directory |
|
@samsonasik just opened #3672, it would be awesome if you can have a look and give your feedback :) |
|
@samsonasik meanwhile the discussion get finalized in #3672 , let's move forward on this one as it's aligned with the current maintainers vision :) |
|
@samsonasik marked as ready for review, as I would like to advance on #3614 so that I can benefit from a reliable cache in my day to day work :) |
|
fwiw, we also experienced problems with a stale cache in our daily work. |
|
Thank you @yguedidi |
|
@staabm I'm on it! soon expect it to be fixed :) |
|
I wonder if we can improve this experience 🙂 This stmts rule can have custom interface check, right? |
|
@TomasVotruba this test make sure that multiple runs can still apply, on use case extract one statement to multi statements, then add new line between them, which the the line information is only can be fetched after rector done, which expected. |
@TomasVotruba It's not clear to me what you mean here, sorry. Would #3672 proposed PR be a better experience in your opinion? :) |
second prerequisite for #3614