Notify in CI when a slow full analysis ran without a pre-existing result cache#5954
Notify in CI when a slow full analysis ran without a pre-existing result cache#5954phpstan-bot wants to merge 4 commits into
Conversation
…ult cache - Add `ResultCacheManager::resultCacheExists()` to report whether the result cache file was on disk when PHPStan started, before `restore()` can unlink it. - Thread that fact through `AnalyseApplication` into a new `AnalysisResult::resultCacheExisted()` getter (defaults to true so a stale but present cache that forced a full re-analysis does not trigger the message). - In `AnalyseCommand`, after a full analysis, print a hint to persist the result cache directory when: running in CI (detected via `CiDetector`), analysing full paths (not `--only-files`), the cache file did not exist at start, and the run took longer than `RESULT_CACHE_CI_NOTIFICATION_ELAPSED_LIMIT` (60s). - The message is intentionally suppressed when the cache existed but was invalid.
Add the notifyAboutMissingResultCacheInCi feature toggle (off by default, on under bleedingEdge.neon) and only emit the missing-result-cache CI hint when it is enabled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@staabm I'd do it for everyone, not just bleeding edge (it doesn't break anything, doesn't make sense). I wouldn't do this kind of test in tests/, I hate these. Instead, I'd make the time configurable and do some E2E tests in e2e-tests.yml. Also, the message should change to have more "call-to-action". Everyone knows it "took more than one minute" but it should say that the CI runs could be much faster. Also would be nice to pair this with phpstan/phpstan#8599. |
staabm
left a comment
There was a problem hiding this comment.
@phpstan-bot adjust the PR according to feedback in #5954 (comment)
Also would be nice to pair this with phpstan/phpstan#8599.
do not work on this item. we will work on it in a separate PR
…ld configurable Drops the bleeding-edge-only feature toggle so the slow-full-analysis hint runs for all users, and replaces the hardcoded 60s constant with a resultCacheCiNotificationSeconds parameter. The message is reworded to be a clearer call to action about speeding up CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds an e2e/result-cache-ci-notification case driven from e2e-tests.yml: a fresh run emits the hint, the cached re-run does not. Replaces the removed CommandTester-based unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed the review feedback from ondrejmirtes' comment. Here's what changed: 1. Run for everyone, not just bleeding edgeRemoved the 2. Made the time configurableReplaced the hardcoded 3. Replaced the
|
Summary
In long-running CI pipelines that do not persist PHPStan's result cache, every run is a full analysis and can be slow. This adds a DX hint that tells the user to persist the result cache directory when a full run was slow and no result cache existed at start.
The message is shown only when all of these hold:
OndraM\CiDetector)--only-files)It is deliberately not shown when the result cache existed but was invalid (stale metadata, expired, corrupted, changed extension file, …) and therefore triggered a full re-analysis.
Changes
src/Analyser/ResultCache/ResultCacheManager.php: addresultCacheExists()which checksis_file($this->cacheFilePath). It is queried beforerestore()so it is unaffected by the@unlink()callsrestore()performs on a corrupted cache.src/Command/AnalyseApplication.php: captureresultCacheManager->resultCacheExists()before restoring and pass it toAnalysisResult.src/Command/AnalysisResult.php: new constructor parameterresultCacheExisted(defaults totrue) plusresultCacheExisted()getter; propagated inwithFileSpecificErrors().src/Command/AnalyseCommand.php: newRESULT_CACHE_CI_NOTIFICATION_ELAPSED_LIMITconstant andreportMissingResultCacheInCi(), called at the end of a full analysis; preserve the new flag when theAnalysisResultis rebuilt for the internal-errors path.Root cause
This is a feature request, not a bug. The key distinction the issue asks for — "cache did not exist" vs "cache existed but was invalid" — was not expressible:
AnalysisResultonly knew whether the cache was used (isResultCacheUsed()), which is false in both cases. The fix adds the missing piece of information (did the cache file exist at start) and surfaces it where the decision is made. Capturing it beforerestore()matters becauserestore()deletes a corrupted cache file, which would otherwise make a present-but-invalid cache look like it never existed.Test
tests/PHPStan/Command/ResultCacheCiNotificationTest.phpdrivesAnalyseCommandviaCommandTesterin an isolated temp working directory (owntmpDir), forcing the elapsed-time condition by passing ananalysisStartTime120 seconds in the past and toggling theGITHUB_ACTIONSenv var:The reported scenario is the only construct on this axis: the baseline-generation and PHPStan Pro paths return before reaching this code (and Pro is blocked in CI anyway), and the internal-errors path intentionally skips the hint. No analogous sibling paths needed the same change.
Fixes phpstan/phpstan#14881