From 239b7569015d9b374b5625995b155a19f327ded3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Bok?= Date: Sun, 18 Jun 2023 21:24:07 +0200 Subject: [PATCH 1/3] [cache] Cache unchanged files on --dry-run v2 --- .../workflows/e2e_consecutive_changes.yaml | 14 ++++++++-- .../expected-output-1-dry-run.diff | 27 +++++++++++++++++++ .../expected-output-2-dry-run.diff | 27 +++++++++++++++++++ ...d-output-1.diff => expected-output-3.diff} | 0 ...d-output-2.diff => expected-output-4.diff} | 0 e2e/e2eTestChangingRunnerWithCache.php | 4 +++ packages/Parallel/WorkerRunner.php | 2 +- phpstan.neon | 1 + 8 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 e2e/consecutive-changes-with-cache/expected-output-1-dry-run.diff create mode 100644 e2e/consecutive-changes-with-cache/expected-output-2-dry-run.diff rename e2e/consecutive-changes-with-cache/{expected-output-1.diff => expected-output-3.diff} (100%) rename e2e/consecutive-changes-with-cache/{expected-output-2.diff => expected-output-4.diff} (100%) diff --git a/.github/workflows/e2e_consecutive_changes.yaml b/.github/workflows/e2e_consecutive_changes.yaml index f3be9164276..f280812c2e1 100644 --- a/.github/workflows/e2e_consecutive_changes.yaml +++ b/.github/workflows/e2e_consecutive_changes.yaml @@ -43,10 +43,20 @@ jobs: run: composer install --ansi working-directory: ${{ matrix.directory }} + # run e2e test with dry run, we expect non-zero exit code + - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-1-dry-run.diff --dry-run + working-directory: ${{ matrix.directory }} + continue-on-error: true + + # run e2e test with dry run once more, we expect non-zero exit code again + - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-2-dry-run.diff --dry-run + working-directory: ${{ matrix.directory }} + continue-on-error: true + # run e2e test - - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-1.diff + - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-3.diff working-directory: ${{ matrix.directory }} # this tests that a 2nd run with cache and consecutive changes works, see https://github.com/rectorphp/rector-src/pull/3614#issuecomment-1507742338 - - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-2.diff + - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-4.diff working-directory: ${{ matrix.directory }} diff --git a/e2e/consecutive-changes-with-cache/expected-output-1-dry-run.diff b/e2e/consecutive-changes-with-cache/expected-output-1-dry-run.diff new file mode 100644 index 00000000000..dd6d38d8b16 --- /dev/null +++ b/e2e/consecutive-changes-with-cache/expected-output-1-dry-run.diff @@ -0,0 +1,27 @@ +1 file with changes +=================== + +1) src/Source.php:1 + + ---------- begin diff ---------- +@@ @@ + + $a = true; + $b = true; +- +-if ($a && $b) { +- return true; ++if (!$a) { ++ return; + } ++if (!$b) { ++ return; ++} ++return true; + ----------- end diff ----------- + +Applied rules: + * ChangeAndIfToEarlyReturnRector + + + [OK] 1 file would have changed (dry-run) by Rector diff --git a/e2e/consecutive-changes-with-cache/expected-output-2-dry-run.diff b/e2e/consecutive-changes-with-cache/expected-output-2-dry-run.diff new file mode 100644 index 00000000000..dd6d38d8b16 --- /dev/null +++ b/e2e/consecutive-changes-with-cache/expected-output-2-dry-run.diff @@ -0,0 +1,27 @@ +1 file with changes +=================== + +1) src/Source.php:1 + + ---------- begin diff ---------- +@@ @@ + + $a = true; + $b = true; +- +-if ($a && $b) { +- return true; ++if (!$a) { ++ return; + } ++if (!$b) { ++ return; ++} ++return true; + ----------- end diff ----------- + +Applied rules: + * ChangeAndIfToEarlyReturnRector + + + [OK] 1 file would have changed (dry-run) by Rector diff --git a/e2e/consecutive-changes-with-cache/expected-output-1.diff b/e2e/consecutive-changes-with-cache/expected-output-3.diff similarity index 100% rename from e2e/consecutive-changes-with-cache/expected-output-1.diff rename to e2e/consecutive-changes-with-cache/expected-output-3.diff diff --git a/e2e/consecutive-changes-with-cache/expected-output-2.diff b/e2e/consecutive-changes-with-cache/expected-output-4.diff similarity index 100% rename from e2e/consecutive-changes-with-cache/expected-output-2.diff rename to e2e/consecutive-changes-with-cache/expected-output-4.diff diff --git a/e2e/e2eTestChangingRunnerWithCache.php b/e2e/e2eTestChangingRunnerWithCache.php index e7560be27aa..8ec3dae87e6 100644 --- a/e2e/e2eTestChangingRunnerWithCache.php +++ b/e2e/e2eTestChangingRunnerWithCache.php @@ -25,6 +25,10 @@ $expectedDiff = 'expected-output.diff'; } +if (isset($argv[3]) && $argv[3] === '--dry-run') { + $e2eCommand .= ' --dry-run'; +} + exec($e2eCommand, $output, $exitCode); $output = trim(implode("\n", $output)); $output = str_replace(__DIR__, '.', $output); diff --git a/packages/Parallel/WorkerRunner.php b/packages/Parallel/WorkerRunner.php index 3a363826fcf..ebb6f04c7da 100644 --- a/packages/Parallel/WorkerRunner.php +++ b/packages/Parallel/WorkerRunner.php @@ -95,7 +95,7 @@ public function run(Encoder $encoder, Decoder $decoder, Configuration $configura if ($errorAndFileDiffs[Bridge::SYSTEM_ERRORS] !== []) { $this->invalidateFile($file); - } elseif (! $configuration->isDryRun()) { + } else if (! $configuration->isDryRun() || $errorAndFileDiffs[Bridge::FILE_DIFFS] === []) { $this->changedFilesDetector->cacheFileWithDependencies($file->getFilePath()); } } catch (Throwable $throwable) { diff --git a/phpstan.neon b/phpstan.neon index b6b3e64332f..0648f902fed 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -56,6 +56,7 @@ parameters: - tests/DependencyInjection/config ignoreErrors: + - '#Cognitive complexity for "Rector\\Parallel\\WorkerRunner\:\:run\(\)" is 12, keep it under 11#' - '#Cognitive complexity for "Rector\\Php80\\NodeResolver\\SwitchExprsResolver\:\:resolve\(\)" is (.*?), keep it under 11#' - From 1b6403048cee41c40148035a66585d2b273ea1b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Bok?= Date: Mon, 19 Jun 2023 18:20:37 +0200 Subject: [PATCH 2/3] [cache] Make --dry-run work with disabled parallelization + add e2e test --- .github/workflows/e2e_consecutive_changes.yaml | 11 ++++++++++- e2e/consecutive-changes-with-cache/rector.php | 4 ++++ phpstan.neon | 1 + src/Application/ApplicationFileProcessor.php | 2 +- 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e_consecutive_changes.yaml b/.github/workflows/e2e_consecutive_changes.yaml index f280812c2e1..53e7438c6ee 100644 --- a/.github/workflows/e2e_consecutive_changes.yaml +++ b/.github/workflows/e2e_consecutive_changes.yaml @@ -22,10 +22,11 @@ jobs: fail-fast: false matrix: php_version: ['8.1'] + rector_disable_parallel: ['true', 'false'] directory: - 'e2e/consecutive-changes-with-cache' - name: End to end test - ${{ matrix.directory }} + name: End to end test - ${{ matrix.directory }} [disableParallel=${{ matrix.rector_disable_parallel }}] steps: - uses: actions/checkout@v3 @@ -47,16 +48,24 @@ jobs: - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-1-dry-run.diff --dry-run working-directory: ${{ matrix.directory }} continue-on-error: true + env: + RECTOR_DISABLE_PARALLEL: ${{ matrix.rector_disable_parallel }} # run e2e test with dry run once more, we expect non-zero exit code again - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-2-dry-run.diff --dry-run working-directory: ${{ matrix.directory }} continue-on-error: true + env: + RECTOR_DISABLE_PARALLEL: ${{ matrix.rector_disable_parallel }} # run e2e test - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-3.diff working-directory: ${{ matrix.directory }} + env: + RECTOR_DISABLE_PARALLEL: ${{ matrix.rector_disable_parallel }} # this tests that a 2nd run with cache and consecutive changes works, see https://github.com/rectorphp/rector-src/pull/3614#issuecomment-1507742338 - run: php ../e2eTestChangingRunnerWithCache.php -o expected-output-4.diff working-directory: ${{ matrix.directory }} + env: + RECTOR_DISABLE_PARALLEL: ${{ matrix.rector_disable_parallel }} diff --git a/e2e/consecutive-changes-with-cache/rector.php b/e2e/consecutive-changes-with-cache/rector.php index f8ea9dfeb9a..7f54b5ae70d 100644 --- a/e2e/consecutive-changes-with-cache/rector.php +++ b/e2e/consecutive-changes-with-cache/rector.php @@ -10,6 +10,10 @@ return static function (RectorConfig $rectorConfig): void { $rectorConfig->cacheClass(FileCacheStorage::class); + if (getenv('RECTOR_DISABLE_PARALLEL') === 'true') { + $rectorConfig->disableParallel(); + } + $rectorConfig->paths([ __DIR__ . '/src', ]); diff --git a/phpstan.neon b/phpstan.neon index 0648f902fed..ab8dab6fe31 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -57,6 +57,7 @@ parameters: ignoreErrors: - '#Cognitive complexity for "Rector\\Parallel\\WorkerRunner\:\:run\(\)" is 12, keep it under 11#' + - '#Cognitive complexity for "Rector\\Core\\Application\\ApplicationFileProcessor\:\:processFiles\(\)" is 13, keep it under 11#' - '#Cognitive complexity for "Rector\\Php80\\NodeResolver\\SwitchExprsResolver\:\:resolve\(\)" is (.*?), keep it under 11#' - diff --git a/src/Application/ApplicationFileProcessor.php b/src/Application/ApplicationFileProcessor.php index 36a672ebb51..62c4f2db7ca 100644 --- a/src/Application/ApplicationFileProcessor.php +++ b/src/Application/ApplicationFileProcessor.php @@ -133,7 +133,7 @@ public function processFiles(array $files, Configuration $configuration): array if ($systemErrorsAndFileDiffs[Bridge::SYSTEM_ERRORS] !== []) { $this->changedFilesDetector->invalidateFile($file->getFilePath()); - } elseif (! $configuration->isDryRun()) { + } else if (! $configuration->isDryRun() || $systemErrorsAndFileDiffs[Bridge::FILE_DIFFS] === []) { $this->changedFilesDetector->cacheFileWithDependencies($file->getFilePath()); } From f7bba1ad50ebfac2a599d7b53478d4623460ff03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Bok?= Date: Mon, 19 Jun 2023 18:28:46 +0200 Subject: [PATCH 3/3] [cache] Fix CS --- packages/Parallel/WorkerRunner.php | 2 +- src/Application/ApplicationFileProcessor.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/Parallel/WorkerRunner.php b/packages/Parallel/WorkerRunner.php index ebb6f04c7da..46a8e4201d9 100644 --- a/packages/Parallel/WorkerRunner.php +++ b/packages/Parallel/WorkerRunner.php @@ -95,7 +95,7 @@ public function run(Encoder $encoder, Decoder $decoder, Configuration $configura if ($errorAndFileDiffs[Bridge::SYSTEM_ERRORS] !== []) { $this->invalidateFile($file); - } else if (! $configuration->isDryRun() || $errorAndFileDiffs[Bridge::FILE_DIFFS] === []) { + } elseif (! $configuration->isDryRun() || $errorAndFileDiffs[Bridge::FILE_DIFFS] === []) { $this->changedFilesDetector->cacheFileWithDependencies($file->getFilePath()); } } catch (Throwable $throwable) { diff --git a/src/Application/ApplicationFileProcessor.php b/src/Application/ApplicationFileProcessor.php index 62c4f2db7ca..30dfbb8a371 100644 --- a/src/Application/ApplicationFileProcessor.php +++ b/src/Application/ApplicationFileProcessor.php @@ -133,7 +133,7 @@ public function processFiles(array $files, Configuration $configuration): array if ($systemErrorsAndFileDiffs[Bridge::SYSTEM_ERRORS] !== []) { $this->changedFilesDetector->invalidateFile($file->getFilePath()); - } else if (! $configuration->isDryRun() || $systemErrorsAndFileDiffs[Bridge::FILE_DIFFS] === []) { + } elseif (! $configuration->isDryRun() || $systemErrorsAndFileDiffs[Bridge::FILE_DIFFS] === []) { $this->changedFilesDetector->cacheFileWithDependencies($file->getFilePath()); }