Skip to content

Update the Rector integration test to a newer commit - #15156

Open
SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:bump-rector-integration
Open

Update the Rector integration test to a newer commit#15156
SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:bump-rector-integration

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

Both Rector jobs (Integration - Rector tests and, separately, the downgrade one) have been red on every phpstan-src pull request since 2.2.10. For rector-src the 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 touches CallCollectionAnalyzer and PHPStanNodeScopeResolver.

The pin here is b14e0ab4 from 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

  • bump the rector-src ref at both call sites to c8f5daef
  • refresh e2e/integration/rector-composer.lock against it, generated with PHP 8.4 since that is what the jobs run

Verified

Replayed the job from the committed lock at the new ref. The two rule tests that were failing in CI now pass:

RemoveUnusedPrivateMethodRector + SimplifyEmptyCheckOnEmptyArrayRector
OK (63 tests, 66 assertions)

The full suite locally leaves three Tests\Bin\RectorTest::testConsoleOutput failures, but those shell out to bin/rector and 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 its Tests workflow, so I am fairly confident they pass in a normal environment. Your CI will settle it either way.

Scope

Only the rector-src pin. rector-downgrade-php is 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.

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>
@SanderMuller

Copy link
Copy Markdown
Contributor Author

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: RemoveUnusedPrivateMethodRector and SimplifyEmptyCheckOnEmptyArrayRector both pass at the new ref. But three Tests\Bin\RectorTest::testConsoleOutput cases now fail, so the job is still red, just for different reasons than before.

Those three shell out to bin/rector and compare its console output. Two of them expect PHPStan's config validation to print Unexpected item 'parameters › invalidParameters' and get empty output instead; the third expects an unreplaced @package_version@.

What I can rule out:

  • not a PHP version thing: Rector's own CI runs 8.4 and 8.5 and is green on the exact commit I pinned
  • not the pin being too new: tests/Bin already exists at their PHPStan 2.2.10 fix, so there is no window that has the fix without these tests

What is left is the phar swap. Rector's CI runs against a composer-installed phpstan, whereas this job copies phpstan.phar, phpstan and bootstrap.php in from the repo, and the assertions that fail are precisely the ones that depend on PHPStan's own error output. So it looks like a real interaction between Rector HEAD and the current 2.2.x phar rather than anything the pin can fix.

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.

@staabm

staabm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

seems related to rectorphp/rector-downgrade-php#394

@SanderMuller

Copy link
Copy Markdown
Contributor Author

Confirmed, and it explains the other job rather than this one.

The downgrade job's single failure is DowngradeHashAlgorithmXxHashRectorTest data set #3, fixture skip_check_phpversion_ternary.php.inc: hash('xxh128', $value) under a PHP_VERSION_ID >= 80100 guard rewritten to md5. That is what rectorphp/rector-downgrade-php#394 fixes, and the pin is 13 commits behind it.

I replayed both downgrade jobs at 1beef93 with the current 2.2.x phar copied in the way the jobs do it: phpunit gives 647 tests, 2 skipped, no failures (635 tests and 1 failure at the old pin), and analyse -c ../rector-downgrade-php.neon reports no errors with the existing baseline entry still matching. It also needs a regenerated rector-downgrade-php-composer.lock, since composer.json moved in between (PHPUnit 13, type-perfect swapped for type-coverage) and both downgrade jobs read that one lock. Put up as #15158, since it is a different job from this one.

It does not help the three tests/Bin failures here. c8f5daef is rector-src main HEAD as of today, so there is no newer pin to move to for those.

@staabm

staabm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

It does not help the three tests/Bin failures here. c8f5daef is rector-src main HEAD as of today, so there is no newer pin to move to for those.

does this mean these tests are also red on the rector repo itself?

@SanderMuller

Copy link
Copy Markdown
Contributor Author

No, they are green there, and chasing that turned up that my phar explanation above was wrong.

On c8f5daef in rector-src: PHP 8.4 tests (ubuntu-latest) and PHP 8.5 tests (ubuntu-latest) both pass, same OS and PHP as this job, and tests/Bin is part of that suite. The only red check on the commit is pick-and-fix, which is not a test job.

I then replayed our job locally: rector-src at c8f5daef, our rector-composer.lock, and phpstan.phar, phpstan, bootstrap.php and turbo-ext copied into vendor/phpstan/phpstan the way the job does it. tests/Bin is 3 of 3 green, with the turbo extension actually loading. So "an interaction between Rector HEAD and the current 2.2.x phar" does not hold, and I should not have written it without reproducing it.

Reading the CI failures properly also changes what they are. All three expectations get '', including the --version one:

1) ...testConsoleOutput@Version with data ('/usr/bin/php8.4 bin/rector --version', 'Rector @package_version@\n')
-'Rector @package_version@
-'
+''

bin/rector --version never touches PHPStan's config validation, so this is not a changed message. The spawned bin/rector writes nothing to stdout in all three cases, which means the subprocess is failing. RectorTest asserts on getOutput() only, so whatever it printed on stderr, and its exit code, are invisible in the log. It is one problem rather than three.

The cheapest way to get the cause is a diagnostic in the job: run php bin/rector --version; echo "exit=$?" before phpunit, once, and read the stderr. Happy to push that here if you want it, or to close this PR and leave the pin bump for whoever picks that up.

@staabm

staabm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

RectorTest asserts on getOutput() only, so whatever it printed on stderr, and its exit code, are invisible in the log. It is one problem rather than three

Does this mean rector-src should be fixed to get more meaningful debug info?

@SanderMuller

Copy link
Copy Markdown
Contributor Author

Yes, and it is a small one. assertSame() takes a message, so the failure can carry the two things it currently drops:

$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: exit code 0, stderr:. On our red job that line is the cause.

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.

@staabm

staabm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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.

@SanderMuller

Copy link
Copy Markdown
Contributor Author

Sent: rectorphp/rector-src#8440.

One detail I changed from the snippet above: getExitCode() is nullable, so it renders with %s and a fallback instead of %d, which would print a misleading 0 for a process that never started. tests/Bin stays green, and their own ecs and phpstan are clean on the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants