Update the Rector integration test to a newer commit - #15156
Update the Rector integration test to a newer commit#15156SanderMuller wants to merge 2 commits into
Conversation
|
CI has answered the caveat in the description, and not in my favour, so here is where this stands. The bump does remove the two rule-level failures: Those three shell out to What I can rule out:
What is left is the phar swap. Rector's CI runs against a composer-installed phpstan, whereas this job copies Happy to close this if you would rather not carry a partial fix, or leave it open as the first half if you think the console-output difference is worth a look on our side. I did not want to leave the description claiming this greens the job when it does not. |
|
seems related to rectorphp/rector-downgrade-php#394 |
|
Confirmed, and it explains the other job rather than this one. The downgrade job's single failure is I replayed both downgrade jobs at It does not help the three |
does this mean these tests are also red on the rector repo itself? |
|
No, they are green there, and chasing that turned up that my phar explanation above was wrong. On I then replayed our job locally: rector-src at Reading the CI failures properly also changes what they are. All three expectations get
The cheapest way to get the cause is a diagnostic in the job: run |
Does this mean rector-src should be fixed to get more meaningful debug info? |
|
Yes, and it is a small one. $this->assertSame($expectedOutput, preg_replace('/ +/', ' ', $process->getOutput()), sprintf(
'exit code %d, stderr: %s',
(int) $process->getExitCode(),
$process->getErrorOutput()
));I tried it against a deliberately wrong expectation, and the failure gains one line above the diff: Happy to send it to rector-src. It is worth as much to them as to us, since any failure of a spawned process in that test currently reports as an empty string with no reason attached. |
yes, thats what I had in mind. |
|
Sent: rectorphp/rector-src#8440. One detail I changed from the snippet above: |
Both Rector jobs have been failing since 2.2.10. The pinned b14e0ab4 is from 2026-08-05 and predates Rector's adaptation to the node callback ordering that changed in that release: rectorphp/rector-src#8420, "Support PHPStan 2.2.10 scope changes", landed 2026-08-30 and updated CallCollectionAnalyzer and PHPStanNodeScopeResolver. Bumping the pin past that commit and refreshing rector-composer.lock against it, generated with PHP 8.4 since that is what the jobs run. Verified by replaying the job from the committed lock: the two rule tests that were failing, RemoveUnusedPrivateMethodRector and SimplifyEmptyCheckOnEmptyArrayRector, are OK (63 tests, 66 assertions). Only the rector-src pin is touched here. The separate rector-downgrade-php job has its own pin and its own failure, which I have not looked into. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3d21c49 to
9edd131
Compare
|
@SanderMuller one rector test still fails. might need another update? |
rectorphp/rector-src#8440 landed on 2026-09-04 and makes the tests/Bin assertion carry the spawned process's exit code and stderr. The previous pin predates it, so the three failures report only an empty string with no reason attached. 63a2f160d is rector-src main HEAD; its own PHP 8.4 and 8.5 ubuntu test jobs are green. rector-composer.lock regenerated against it on PHP 8.4, the version the jobs run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Yes, and the update to make is the pin, so that the failure finally says why. The three Bumped both pin sites to Replayed the job at the new pin, with the phar,
I still cannot reproduce the CI failure locally, and there is one hypothesis I cannot rule out. My One note for anyone replaying the analyse half: it reports 11 errors on PHP 8.5 and 0 on 8.4. I nearly wrote up "the bump breaks the analyse job" before checking on the version the job actually uses. |
Both Rector jobs (
Integration - Rector testsand, separately, the downgrade one) have been red on every phpstan-src pull request since 2.2.10. Forrector-srcthe cause is just a stale pin.The node callback ordering changed in 2.2.10 (
ae03ab3ec, part of phpstan/phpstan-src#6248). Rector attaches scopes to nodes as callbacks arrive and reads them back later, so it needed updating, which it got in rectorphp/rector-src#8420, "Support PHPStan 2.2.10 scope changes", landed 2026-08-30. It touchesCallCollectionAnalyzerandPHPStanNodeScopeResolver.The pin here is
b14e0ab4from 2026-08-05, three and a half weeks before that fix, so the job has been testing a Rector that could not pass.The change
rector-srcref at both call sites toc8f5daefe2e/integration/rector-composer.lockagainst it, generated with PHP 8.4 since that is what the jobs runVerified
Replayed the job from the committed lock at the new ref. The two rule tests that were failing in CI now pass:
The full suite locally leaves three
Tests\Bin\RectorTest::testConsoleOutputfailures, but those shell out tobin/rectorand compare its console output, and the expectation data in my run contains my own PHP binary path. Rector's own CI is green on this exact commit today, including itsTestsworkflow, so I am fairly confident they pass in a normal environment. Your CI will settle it either way.Scope
Only the
rector-srcpin.rector-downgrade-phpis a separate repository with its own pin and its own single failure, which I have not diagnosed, so I left it alone.Context: #15155, where you pointed out that callback order was never promised and this belonged on the Rector side. Agreed, and it turned out they had already done it.