From 39137b02227433d2e441be2c71c8852c35c5e820 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 10 Oct 2026 22:57:36 +0700 Subject: [PATCH 01/13] perf(tests): read git history once for lastmod, render SSR only in the ssr group --- .github/workflows/tests.yml | 19 +-- CLAUDE.md | 5 +- app/Support/Seo/LastModified.php | 85 ++++++------ phpunit.xml | 1 + tests/Feature/Assets/AssetManifestTest.php | 2 +- tests/Feature/Assets/EmptySlotsTest.php | 2 +- tests/Feature/Chrome/LayoutRegressionTest.php | 4 +- tests/Feature/Chrome/PageChunkTest.php | 12 +- tests/Feature/Chrome/PageFrameTest.php | 18 +-- tests/Feature/Chrome/SiteChromeTest.php | 34 ++--- tests/Feature/Compare/ComparePagesTest.php | 18 +-- .../Feature/Content/PositioningGuardsTest.php | 4 +- tests/Feature/Databases/DatabasePagesTest.php | 20 +-- tests/Feature/Features/FeaturePagesTest.php | 8 +- .../Integrations/IntegrationPagesTest.php | 4 +- tests/Feature/Landing/AboutPageTest.php | 10 +- .../Feature/Landing/AnalyticsConsentTest.php | 4 +- tests/Feature/Landing/BlogFeedTest.php | 2 +- tests/Feature/Landing/BlogTest.php | 18 +-- tests/Feature/Landing/BrandPageTest.php | 2 +- tests/Feature/Landing/FaqPageTest.php | 2 +- tests/Feature/Landing/HomepageRenderTest.php | 32 ++--- tests/Feature/Landing/InternalLinksTest.php | 2 +- tests/Feature/Landing/IosPageTest.php | 8 +- tests/Feature/Landing/LandingSeoTest.php | 10 +- tests/Feature/Landing/LanguageBarTest.php | 2 +- tests/Feature/Landing/LegalPagesTest.php | 2 +- tests/Feature/Landing/LicenseFeaturesTest.php | 2 +- tests/Feature/Landing/SecurityPageTest.php | 2 +- tests/Feature/Landing/TopBannerTest.php | 4 +- .../Feature/Localization/ForeignTextTest.php | 4 +- .../Localization/HomeSentencesTest.php | 2 +- .../Localization/LocaleRoutingTest.php | 2 +- .../Feature/Localization/RootTemplateTest.php | 3 +- tests/Feature/Localization/TypographyTest.php | 12 +- tests/Feature/Pricing/PricingPageTest.php | 12 +- tests/Feature/Pricing/RegionalPricingTest.php | 2 +- tests/Feature/Releases/DownloadPageTest.php | 12 +- .../Security/ContentSecurityPolicyTest.php | 4 +- tests/Feature/Seo/HreflangReciprocityTest.php | 2 +- tests/Feature/Seo/RedirectsTest.php | 2 +- tests/Feature/SsrServerTest.php | 2 +- tests/Feature/Theme/ThemeControlTest.php | 2 +- tests/Feature/ThirdParty/ScriptsTest.php | 6 +- tests/Support/ssr.php | 125 ++++++------------ 45 files changed, 245 insertions(+), 285 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index eabc4984..c8244264 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -63,7 +63,7 @@ jobs: run: php artisan assets:generate --check - name: Tests - run: ./vendor/bin/pest + run: ./vendor/bin/pest --exclude-group=ssr frontend: runs-on: ubuntu-26.04 @@ -95,11 +95,8 @@ jobs: - name: Build run: npm run build - # Every case behind `requireSsr()` (tests/Support/ssr.php) needs a built SSR - # bundle and a running SSR service, and skips itself when either is missing. - # The `php` job above provides neither, so those cases skip there. This job - # is the one place they execute; REQUIRE_SSR turns those skips into failures - # so they cannot go quiet again. + # The ssr group: tests that need the built bundles, node_modules or a live SSR + # service. REQUIRE_SSR turns their skips into failures here. ssr: runs-on: ubuntu-26.04 @@ -168,18 +165,10 @@ jobs: cat /tmp/ssr.log >&2 || true exit 1 - # The whole suite, not one file. - # - # This once ran only HomepageRenderTest.php, which left every other - # SSR-rendered assertion skipping on every push: the `php` job above - # builds no bundles, so every `ssrHtml()` / `requireSsr()` assertion - # short-circuits there, and this job is the one place they execute. - # - # REQUIRE_SSR turns a skip into a failure, so this cannot go quiet again. - name: SSR render tests env: REQUIRE_SSR: 1 - run: ./vendor/bin/pest + run: ./vendor/bin/pest --group=ssr - name: SSR service log if: always() diff --git a/CLAUDE.md b/CLAUDE.md index 5531a3d6..94701969 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -137,8 +137,9 @@ Every change needs a test. Feature tests live in `tests/Feature`; there is no `RefreshDatabase` anywhere and there must not be — there is no database. No test reaches the network: `tests/Pest.php` prevents stray HTTP requests, so -fake GitHub with `Http::fake()`. Tests that assert on rendered markup go behind -`requireSsr()` and skip locally without a built SSR bundle. +fake GitHub with `Http::fake()`. SSR is off in tests: a test that asserts on +rendered markup calls `requireSsr()` and sits in `->group('ssr')`, which CI runs +in the job with a live SSR service. It skips locally without one. ## Content diff --git a/app/Support/Seo/LastModified.php b/app/Support/Seo/LastModified.php index 2187b47e..dabdab83 100644 --- a/app/Support/Seo/LastModified.php +++ b/app/Support/Seo/LastModified.php @@ -7,32 +7,16 @@ use Symfony\Component\Process\Process; use Throwable; -/** - * When the files behind a page last changed, for the sitemap's `lastmod`. - * - * Git first: `git log -1 --format=%cI -- ` is the last commit that - * touched any of them. File mtimes are no good on their own, because every - * `git pull` on the server resets them to the pull time, and a sitemap whose - * every `lastmod` moves on each deploy teaches crawlers to ignore the field. - * The newest mtime is the fallback for a checkout without history or files not - * committed yet, and a page with neither gets no `lastmod` at all rather than - * a made-up "now". - * - * A page's sources can include other locales' copy. `forEntry()` keeps only - * what drives the page in the locale asked for: the shared data files plus - * that locale's own content, legal or blog file. - */ +// The sitemap's lastmod: the last commit touching a page's sources. Mtimes are +// only a fallback, since every `git pull` on the server resets them. final class LastModified { - /** - * Seconds `git log` may take before the mtime fallback is used. - */ - private const GIT_TIMEOUT = 2.0; + private const GIT_TIMEOUT = 10.0; /** - * @var array + * @var array|null path => [unix time, ISO 8601] of its last commit */ - private array $memo = []; + private ?array $commits = null; public function __construct( private readonly string $root, @@ -48,29 +32,28 @@ public function forEntry(PageEntry $entry, string $locale): ?CarbonImmutable */ public function forPaths(array $paths): ?CarbonImmutable { - $paths = array_values(array_unique($paths)); - sort($paths); - if ($paths === []) { return null; } - $key = implode("\0", $paths); + $this->commits ??= $this->readGitLog(); + $newest = null; + + foreach (array_unique($paths) as $path) { + $commit = $this->commits[$path] ?? null; - if (array_key_exists($key, $this->memo)) { - return $this->memo[$key]; + if ($commit !== null && ($newest === null || $commit[0] > $newest[0])) { + $newest = $commit; + } } - return $this->memo[$key] = $this->fromGit($paths) ?? $this->fromMtime($paths); + return $newest !== null ? CarbonImmutable::parse($newest[1]) : $this->fromMtime($paths); } /** - * The sources that drive a page in one locale. - * - * A source belongs to a locale when one of its directories is named after - * that locale (`content/vi/…`, `legal/en/…`, `blog/vi/…`). A post directly - * under `resources/blog/` belongs to the default locale. Anything else is - * shared data and drives every locale. + * The sources that drive a page in one locale: shared data, plus the files + * under a directory named after that locale. A post directly under + * `resources/blog/` belongs to the default locale. * * @param list $sources * @return list @@ -97,20 +80,40 @@ private static function localeOf(string $source): ?string } /** - * @param list $paths + * @return array */ - private function fromGit(array $paths): ?CarbonImmutable + private function readGitLog(): array { try { - $process = new Process(['git', 'log', '-1', '--format=%cI', '--', ...$paths], $this->root, null, null, self::GIT_TIMEOUT); + $process = new Process( + ['git', '-c', 'core.quotePath=false', 'log', '--format=%x00%ct %cI', '--name-only', '--relative'], + $this->root, + null, + null, + self::GIT_TIMEOUT, + ); $process->run(); + } catch (Throwable) { + return []; + } - $output = trim($process->getOutput()); + if (! $process->isSuccessful()) { + return []; + } - return $process->isSuccessful() && $output !== '' ? CarbonImmutable::parse($output) : null; - } catch (Throwable) { - return null; + $commits = []; + $commit = null; + + foreach (explode("\n", $process->getOutput()) as $line) { + if (str_starts_with($line, "\0")) { + [$time, $iso] = explode(' ', substr($line, 1), 2); + $commit = [(int) $time, $iso]; + } elseif ($line !== '' && $commit !== null && (! isset($commits[$line]) || $commit[0] > $commits[$line][0])) { + $commits[$line] = $commit; + } } + + return $commits; } /** diff --git a/phpunit.xml b/phpunit.xml index ae9923a6..4d7e2403 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -18,6 +18,7 @@ + diff --git a/tests/Feature/Assets/AssetManifestTest.php b/tests/Feature/Assets/AssetManifestTest.php index aa179203..c91511fa 100644 --- a/tests/Feature/Assets/AssetManifestTest.php +++ b/tests/Feature/Assets/AssetManifestTest.php @@ -884,4 +884,4 @@ function (string $root): AssetManifest { /* Under REQUIRE_SSR a skip here would be green; finding nothing means the selector or the markup changed. */ Assert::assertGreaterThan(0, $found, 'No page renders an AssetSlot placeholder'); -}); +})->group('ssr'); diff --git a/tests/Feature/Assets/EmptySlotsTest.php b/tests/Feature/Assets/EmptySlotsTest.php index 57ffe055..deae3761 100644 --- a/tests/Feature/Assets/EmptySlotsTest.php +++ b/tests/Feature/Assets/EmptySlotsTest.php @@ -104,4 +104,4 @@ function emptySlotPages(): array } expect($checked)->toBeGreaterThanOrEqual(count(emptySlotPages())); -}); +})->group('ssr'); diff --git a/tests/Feature/Chrome/LayoutRegressionTest.php b/tests/Feature/Chrome/LayoutRegressionTest.php index 608dea50..d6f4b6c2 100644 --- a/tests/Feature/Chrome/LayoutRegressionTest.php +++ b/tests/Feature/Chrome/LayoutRegressionTest.php @@ -188,7 +188,7 @@ function layoutSource(string $path): string expect($section[0] ?? '')->toContain('class="relative overflow-x-auto') ->not->toContain('min-w-[36rem]'); -}); +})->group('ssr'); it('does not repeat the contact line under a document that has its own Contact section', function (string $path, bool $repeats): void { // The markup only: the catalog line also travels in the page props. @@ -200,7 +200,7 @@ function layoutSource(string $path): string ['/vi/privacy', false], ['/terms', false], ['/refund-policy', true], -]); +])->group('ssr'); it('keeps a busy button focusable, so the reader who pressed it keeps their place', function (): void { /* diff --git a/tests/Feature/Chrome/PageChunkTest.php b/tests/Feature/Chrome/PageChunkTest.php index 4448fc0d..b255944c 100644 --- a/tests/Feature/Chrome/PageChunkTest.php +++ b/tests/Feature/Chrome/PageChunkTest.php @@ -24,6 +24,8 @@ */ function clientManifest(): array { + requireSsrJob(); + $path = public_path('build/manifest.json'); if (! is_file($path)) { @@ -68,7 +70,7 @@ function clientManifest(): array Assert::assertContains($catalog, $entry['dynamicImports'] ?? [], "app.tsx does not load {$catalog} on demand"); Assert::assertNotContains($catalog, $entry['imports'] ?? [], "app.tsx imports {$catalog} statically"); } -}); +})->group('ssr'); it('sends a translated page its own language\'s catalog with the document, and no other', function (string $path, string $locale): void { $manifest = clientManifest(); @@ -87,7 +89,7 @@ function clientManifest(): array ['/vi/download', 'vi'], ['/pt-BR/pricing', 'pt-BR'], ['/zh-Hans', 'zh-Hans'], -]); +])->group('ssr'); it('renders the error page from a file that exists', function (): void { $this->get('/no-such-page')->assertNotFound(); @@ -108,7 +110,7 @@ function clientManifest(): array foreach ($pages as $page) { Assert::assertArrayHasKey($page, $manifest, "{$page} has no entry in public/build/manifest.json"); } -}); +})->group('ssr'); it('sends the page chunk and its imports with the document', function (string $path, string $component): void { $manifest = clientManifest(); @@ -137,7 +139,7 @@ function clientManifest(): array ['/pricing', 'Pricing'], ['/vi/download', 'Download'], ['/blog/tablepro-0-77', 'Blog/Post'], -]); +])->group('ssr'); it('renders a component with no file without asking the manifest for it', function (): void { clientManifest(); @@ -150,4 +152,4 @@ function clientManifest(): array expect($html)->not->toContain('Nope/Missing.tsx') ->toContain('"component":"Nope\/Missing"'); -}); +})->group('ssr'); diff --git a/tests/Feature/Chrome/PageFrameTest.php b/tests/Feature/Chrome/PageFrameTest.php index 6ca69312..d365551d 100644 --- a/tests/Feature/Chrome/PageFrameTest.php +++ b/tests/Feature/Chrome/PageFrameTest.php @@ -68,7 +68,7 @@ function frameCss(): string */ expect((string) file_get_contents(resource_path('js/components/shared/frame-rails.tsx')))->toContain('max-w-[80rem]'); expect((string) file_get_contents(resource_path('js/components/ui/container.tsx')))->toContain("wide: 'max-w-[76rem]'")->toContain('lg:px-8'); -}); +})->group('ssr'); it('joins every two blocks of main with one full-bleed rule', function (string $path): void { $xpath = frameXPath(ssrHtml($path)); @@ -90,7 +90,7 @@ function frameCss(): string Assert::assertDoesNotMatchRegularExpression('/^(?:[a-z]+:)*border-[tby](?:-|$)/', $class, "{$path}: {$name} draws its own rule beside the frame's join"); } } -})->with(frameTemplates()); +})->with(frameTemplates())->group('ssr'); it('draws the join in CSS, by position, and lands anchors behind the header\'s rule', function (): void { expect(frameCss())->toMatch('/main > \* \+ \* \{\s*border-top: 1px solid var\(--rule\);/') @@ -109,7 +109,7 @@ function frameCss(): string expect($xpath->query('//footer[@data-join-mark]')->length)->toBe(1, "{$path}: the footer's rule is the last join and is marked like the others"); expect($xpath->query('//main//*[@data-join-mark]')->length)->toBe(0, "{$path}: joins in
are marked by position, not by a switch"); -})->with(frameTemplates()); +})->with(frameTemplates())->group('ssr'); it('shows marks only where a 1280px frame leaves them room, and never in forced colours or print', function (): void { $css = frameCss(); @@ -141,7 +141,7 @@ function frameCss(): string } expect(true)->toBeTrue(); -})->with(frameTemplates()); +})->with(frameTemplates())->group('ssr'); it('draws cells with one shared line, reaching the rails from 1280px and the screen edge below', function (): void { $css = frameCss(); @@ -168,7 +168,7 @@ function frameCss(): string foreach (['sponsors', 'features', 'safety', 'platforms', 'open-source'] as $id) { expect(frameClasses($xpath->query("//section[@id=\"{$id}\"]")->item(0)))->toContain('pb-0', 'md:pb-0', 'xl:pb-0'); } -})->with(['/', '/vi']); +})->with(['/', '/vi'])->group('ssr'); it('lets the join close a list that ends a block, but never one inside a card', function (): void { expect(frameCss())->toContain('main > * > :is(dl, [data-rule-list]):not(.rounded-panel *):last-child') @@ -221,7 +221,7 @@ function frameCss(): string } expect(true)->toBeTrue(); -})->with(frameTemplates()); +})->with(frameTemplates())->group('ssr'); it('reaches the rails from any measure with container units, and keeps the text where it was', function (): void { $css = frameCss(); @@ -248,7 +248,7 @@ function frameCss(): string 'download' => ['/download', ['install', 'updates', 'older-versions']], 'post' => ['/blog/tablepro-0-77', ['related']], 'blog' => ['/blog', ['guides', 'releases']], -]); +])->group('ssr'); it('sets each FAQ topic beside its questions, as two cells', function (): void { $xpath = frameXPath(ssrHtml('/faq')); @@ -260,7 +260,7 @@ function frameCss(): string expect($xpath->query('./*', $grid)->length)->toBe(2, 'A topic is its heading cell and its questions cell'); expect($xpath->query('.//*[@data-rule-list]', $grid)->item(0)?->getAttribute('class'))->toContain('cell-rows'); } -}); +})->group('ssr'); it('runs each blog index list from rail to rail', function (): void { $xpath = frameXPath(ssrHtml('/blog')); @@ -271,4 +271,4 @@ function frameCss(): string expect($list)->not->toBeNull("#{$id} has no ruled list"); expect(frameClasses($list))->toContain('frame-rows'); } -}); +})->group('ssr'); diff --git a/tests/Feature/Chrome/SiteChromeTest.php b/tests/Feature/Chrome/SiteChromeTest.php index 5c104395..3948b51f 100644 --- a/tests/Feature/Chrome/SiteChromeTest.php +++ b/tests/Feature/Chrome/SiteChromeTest.php @@ -145,7 +145,7 @@ function chromeLocales(): array // Nothing bleeds past the viewport: the ledger's 200vw rules are gone. expect($html)->not->toContain('w-[200vw]'); -})->with('chrome locales'); +})->with('chrome locales')->group('ssr'); it('links the header to the sections in the reader\'s language', function (string $path, string $locale, string $prefix): void { $header = chromeRegion(ssrHtml($path), 'header'); @@ -171,7 +171,7 @@ function chromeLocales(): array } Assert::assertNotNull(chromeLink($header, "{$prefix}/ios", $vi ? 'iPhone và iPad' : 'iPhone & iPad'), 'The platform page is missing from the Features menu'); -})->with('chrome locales'); +})->with('chrome locales')->group('ssr'); it('links GitHub with its star count, in the header row and in the menu', function (string $path, string $locale): void { Storage::fake(StarCount::DISK); @@ -194,7 +194,7 @@ function chromeLocales(): array ->and($bar['attrs'])->not->toHaveKey('target') ->and($menu['text'])->toBe('GitHub↗6.2k') ->and($menu['attrs']['aria-label'])->toBe($name); -})->with('chrome locales'); +})->with('chrome locales')->group('ssr'); it('names the GitHub link by its word until a count is stored', function (string $path): void { Storage::fake(StarCount::DISK); @@ -209,7 +209,7 @@ function chromeLocales(): array ->and($links[0]['attrs'])->not->toHaveKey('aria-label') ->and($links[1]['text'])->toBe('GitHub↗') ->and($links[1]['attrs'])->not->toHaveKey('aria-label'); -})->with(['/download', '/vi/download']); +})->with(['/download', '/vi/download'])->group('ssr'); it('shows the desktop row only where it fits, in every language', function (string $code, string $prefix): void { /* @@ -238,7 +238,7 @@ function chromeLocales(): array expect($header)->toMatch('/