Update the Rector integration test to a newer commit - #15156
Conversation
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>
|
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 (
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.