fix(e2e): stabilize monorepo playwright runs across apps#1660
fix(e2e): stabilize monorepo playwright runs across apps#1660yacosta738 merged 2 commits intomainfrom
Conversation
Deploying yap-portfolio with
|
| Latest commit: |
a04dc71
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2dfd464.yacosta738-github-io.pages.dev |
| Branch Preview URL: | https://fix-e2e-preflight.yacosta738-github-io.pages.dev |
📝 WalkthroughWalkthroughPlaywright configs updated to ignore specific E2E tests and portfolio's baseURL/ports standardized to 4321; package.json test:e2e script changed to run portfolio then blog sequentially. Several non-functional formatting tweaks in content and config files; one alias update in vitest config. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
yap-api | a04dc71 | Commit Preview URL Branch Preview URL |
Feb 23 2026, 06:11 PM |
🤖 CI Pipeline Results
Commit: 77390b8 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
48-48: Sequential execution with&&silently skips blog tests on portfolio failure.The deliberate sequential ordering correctly prevents port/server contention — that design is sound. The minor side-effect is that a portfolio failure leaves blog test status unknown in CI. If independent suite reporting matters, consider:
- "test:e2e": "pnpm test:e2e:portfolio && pnpm test:e2e:blog", + "test:e2e": "pnpm test:e2e:portfolio; pnpm test:e2e:blog",Using
;runs both suites unconditionally and still preserves the exit code of the last command. If fail-fast is intentional, the current&&is fine — just worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 48, The test:e2e script currently uses "pnpm test:e2e:portfolio && pnpm test:e2e:blog" which stops running blog tests if portfolio fails; change the script so both suites always run (e.g., replace && with ; to run them sequentially unconditionally) or implement a runner that executes both and aggregates exit codes; update the "test:e2e" entry to run "pnpm test:e2e:portfolio" then "pnpm test:e2e:blog" unconditionally (or use a small wrapper to run both and return a non-zero exit if either fails) so CI reports both suites even when one fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blog/playwright.config.ts`:
- Line 9: The Playwright config's testIgnore patterns use relative globs that
won't reliably match absolute paths; update the testIgnore arrays in the
Playwright configs (the testIgnore property in apps/blog/playwright.config.ts
and apps/portfolio/playwright.config.ts) to prefix ignored file patterns with
"**/" (e.g., change "tests/e2e/contact-form.spec.ts" to
"**/tests/e2e/contact-form.spec.ts") so the glob will match absolute file paths
as Playwright expects.
---
Nitpick comments:
In `@package.json`:
- Line 48: The test:e2e script currently uses "pnpm test:e2e:portfolio && pnpm
test:e2e:blog" which stops running blog tests if portfolio fails; change the
script so both suites always run (e.g., replace && with ; to run them
sequentially unconditionally) or implement a runner that executes both and
aggregates exit codes; update the "test:e2e" entry to run "pnpm
test:e2e:portfolio" then "pnpm test:e2e:blog" unconditionally (or use a small
wrapper to run both and return a non-zero exit if either fails) so CI reports
both suites even when one fails.
| */ | ||
| export default defineConfig({ | ||
| testDir: "tests/e2e", | ||
| testIgnore: ["tests/e2e/contact-form.spec.ts"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check if contact-form.spec.ts physically exists in apps/blog's test tree
echo "=== Checking for contact-form.spec.ts in apps/blog ==="
fd "contact-form.spec.ts" apps/blog 2>/dev/null || echo "Not found"
# 2. List all .spec.ts files in apps/blog/tests/e2e to see what exists
echo "=== All .spec.ts files in apps/blog/tests/e2e ==="
fd -e "spec.ts" apps/blog/tests/e2e 2>/dev/null || echo "Directory does not exist or no specs found"
# 3. Check if apps/blog/tests/e2e directory exists at all
echo "=== Directory listing for apps/blog/tests ==="
ls -la apps/blog/tests 2>/dev/null || echo "No tests directory in apps/blog"
# 4. Look for the playwright.config.ts file and read its content
echo "=== Current playwright.config.ts content in apps/blog ==="
cat apps/blog/playwright.config.ts 2>/dev/null || echo "File not found"
# 5. Check apps/portfolio's playwright config for comparison
echo "=== Current playwright.config.ts content in apps/portfolio ==="
cat apps/portfolio/playwright.config.ts 2>/dev/null || echo "File not found"
# 6. List all test files in apps/portfolio to see blog-only specs
echo "=== All .spec.ts files in apps/portfolio/tests/e2e ==="
fd -e "spec.ts" apps/portfolio/tests 2>/dev/null || echo "No test files found"Repository: yacosta738/yacosta738.github.io
Length of output: 7353
🌐 Web query:
Playwright testIgnore relative path pattern matching documentation
💡 Result:
testIgnore patterns are matched against the absolute file path of each test file; string values are treated as glob patterns (you can also use RegExp or an array of globs/regexes). So “relative-looking” patterns generally need to be written to match anywhere in the absolute path (e.g., prefix with **/). (playwright.dev)
Examples (from the docs and typical usage):
- Ignore a directory anywhere:
testIgnore: '**/test-assets/**'(playwright.dev) - You can also do:
testIgnore: ['**/e2e/experimental/**', /.*\.flaky\.spec\.(ts|js)$/](glob + regex) (playwright.dev)
Sources
- [1] Playwright API docs —
testConfig.testIgnore(playwright.dev) - [2] Playwright “Test configuration” — filtering tests (
testIgnore/testMatch) (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-testconfig
- 2: https://playwright.dev/docs/api/class-testconfig
- 3: https://playwright.dev/docs/api/class-testconfig
- 4: https://playwright.dev/docs/api/class-testconfig
- 5: https://playwright.dev/docs/test-configuration?utm_source=openai
Fix testIgnore glob patterns to use **/ prefix for reliable absolute-path matching.
Per Playwright's official documentation, testIgnore patterns are matched against the absolute file path using glob semantics. The pattern "tests/e2e/contact-form.spec.ts" (relative form) will not reliably match an absolute path like /runner/work/.../apps/blog/tests/e2e/contact-form.spec.ts without the **/ prefix.
Update both apps/blog/playwright.config.ts and apps/portfolio/playwright.config.ts to use the documented glob format:
Example for apps/blog
- testIgnore: ["tests/e2e/contact-form.spec.ts"],
+ testIgnore: ["**/contact-form.spec.ts"],Example for apps/portfolio
testIgnore: [
- "tests/e2e/comments*.spec.ts",
- "tests/e2e/search.spec.ts",
- "tests/e2e/newsletter.spec.ts",
- "tests/e2e/tag-external-articles.spec.ts",
+ "**/comments*.spec.ts",
+ "**/search.spec.ts",
+ "**/newsletter.spec.ts",
+ "**/tag-external-articles.spec.ts",
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testIgnore: ["tests/e2e/contact-form.spec.ts"], | |
| testIgnore: ["**/contact-form.spec.ts"], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blog/playwright.config.ts` at line 9, The Playwright config's testIgnore
patterns use relative globs that won't reliably match absolute paths; update the
testIgnore arrays in the Playwright configs (the testIgnore property in
apps/blog/playwright.config.ts and apps/portfolio/playwright.config.ts) to
prefix ignored file patterns with "**/" (e.g., change
"tests/e2e/contact-form.spec.ts" to "**/tests/e2e/contact-form.spec.ts") so the
glob will match absolute file paths as Playwright expects.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/portfolio/vitest.config.ts (1)
22-26:⚠️ Potential issue | 🟡 MinorCoverage
includemay no longer align with what the unit tests actually exercise.With the alias now pointing to
packages/shared/src/, code imported via@/in unit tests lives outsideapps/portfolio/src/. The currentcoverage.includeofsrc/**/*.tsandsrc/**/*.astrowill not capture shared-package execution paths, so coverage numbers will undercount what is being exercised (and may overcount uncovered files inapps/portfolio/src/that are no longer reachable via@/).If the portfolio's unit tests are primarily testing shared-package code through the
@/alias, consider also including the shared package source in coverage (or adjusting the include to match the actual tested surface).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portfolio/vitest.config.ts` around lines 22 - 26, The coverage.include in vitest.config.ts currently only targets "src/**/*.ts" and "src/**/*.astro" but tests import shared code through the "@/” alias (pointing at packages/shared/src), so coverage undercounts executed code; update the coverage.include to also include the shared package sources (e.g., packages/shared/src/**/*.ts and any relevant extensions) or otherwise adjust the include globs to match the actual tested surface so files imported via "@/..." are measured; locate the coverage.include setting in apps/portfolio/vitest.config.ts and add the shared package glob(s) (or replace the includes with a combined pattern) to ensure coverage captures shared-package execution paths.
🧹 Nitpick comments (2)
apps/portfolio/vitest.config.ts (2)
11-13: Consider anchoring the resolved path to the config file rather thanprocess.cwd().
path.resolve("../../packages/shared/src")is resolved relative toprocess.cwd(), which isapps/portfolio/only when the script is invoked through the package's own pnpm script. Invoking vitest from the monorepo root (e.g.,vitest run apps/portfolio/...) would produce a wrong path. A file-relative anchor is safer:♻️ Proposed fix using `import.meta.url`
+import { fileURLToPath } from "node:url"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + alias: [ { find: "@/", - replacement: `${path.resolve("../../packages/shared/src")}/`, + replacement: `${path.resolve(__dirname, "../../packages/shared/src")}/`, }, - { find: "@", replacement: path.resolve("../../packages/shared/src") }, + { find: "@", replacement: path.resolve(__dirname, "../../packages/shared/src") }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portfolio/vitest.config.ts` around lines 11 - 13, The current replacement entry uses path.resolve("../../packages/shared/src") which is resolved from process.cwd() and can be incorrect when Vitest is run from the monorepo root; change the resolution to be file-relative by computing the config directory from import.meta.url (e.g. const __dirname = path.dirname(fileURLToPath(import.meta.url))) and then use path.resolve(__dirname, "../../packages/shared/src") for both replacement entries (the one ending with a trailing slash and the one for "@" in the config) so the aliases are always resolved relative to the vitest config file.
9-13: Consider usingimport.meta.urlfor more robust path resolution instead ofpath.resolve()with relative paths.The module imports in unit tests (
@/utils/resolveSiteUrl,@/lib/collection.entity,@/core/resume/*,@/i18n) all resolve correctly—these modules exist only in the shared package, not locally in portfolio/src (which contains only 13 files: pages, layouts, and config). The alias redirect is working as intended. However,path.resolve("../../packages/shared/src")is CWD-dependent; if vitest is invoked from the monorepo root or a different directory, the relative path resolution may break. Usingimport.meta.urlwithfileURLToPath()would provide file-relative resolution that works regardless of where the command is run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portfolio/vitest.config.ts` around lines 9 - 13, The alias entries using path.resolve("../../packages/shared/src") in vitest.config.ts are CWD-dependent; replace them with file-relative resolution using import.meta.url and fileURLToPath so the alias always points to the shared/src directory regardless of where vitest is run. Update the two alias objects (find: "@/"; replacement: ... and find: "@"; replacement: ...) to compute the replacement via const __dirname = path.dirname(fileURLToPath(import.meta.url)) and then resolve the shared/src path from __dirname (or directly use fileURLToPath(new URL(..., import.meta.url))); ensure both alias replacements use this file-relative value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/portfolio/vitest.config.ts`:
- Around line 22-26: The coverage.include in vitest.config.ts currently only
targets "src/**/*.ts" and "src/**/*.astro" but tests import shared code through
the "@/” alias (pointing at packages/shared/src), so coverage undercounts
executed code; update the coverage.include to also include the shared package
sources (e.g., packages/shared/src/**/*.ts and any relevant extensions) or
otherwise adjust the include globs to match the actual tested surface so files
imported via "@/..." are measured; locate the coverage.include setting in
apps/portfolio/vitest.config.ts and add the shared package glob(s) (or replace
the includes with a combined pattern) to ensure coverage captures shared-package
execution paths.
---
Nitpick comments:
In `@apps/portfolio/vitest.config.ts`:
- Around line 11-13: The current replacement entry uses
path.resolve("../../packages/shared/src") which is resolved from process.cwd()
and can be incorrect when Vitest is run from the monorepo root; change the
resolution to be file-relative by computing the config directory from
import.meta.url (e.g. const __dirname =
path.dirname(fileURLToPath(import.meta.url))) and then use
path.resolve(__dirname, "../../packages/shared/src") for both replacement
entries (the one ending with a trailing slash and the one for "@" in the config)
so the aliases are always resolved relative to the vitest config file.
- Around line 9-13: The alias entries using
path.resolve("../../packages/shared/src") in vitest.config.ts are CWD-dependent;
replace them with file-relative resolution using import.meta.url and
fileURLToPath so the alias always points to the shared/src directory regardless
of where vitest is run. Update the two alias objects (find: "@/"; replacement:
... and find: "@"; replacement: ...) to compute the replacement via const
__dirname = path.dirname(fileURLToPath(import.meta.url)) and then resolve the
shared/src path from __dirname (or directly use fileURLToPath(new URL(...,
import.meta.url))); ensure both alias replacements use this file-relative value.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/blog/astro.config.mjsapps/blog/src/content.config.tsapps/blog/src/data/resume/en/resume.jsonapps/blog/src/data/resume/es/resume.jsonapps/blog/src/pages/[lang]/index.astroapps/portfolio/src/content.config.tsapps/portfolio/vitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- apps/portfolio/src/content.config.ts
- apps/blog/src/content.config.ts
- apps/blog/src/data/resume/en/resume.json
- apps/blog/src/data/resume/es/resume.json
🤖 CI Pipeline Results
Commit: f76d8b5 |
1 participant
Summary
apps/portfolioPlaywright base URL and web server ports to4321so it targets the correct app during E2E executiontestIgnore(portfolioignores blog-only specs,blogignores portfolio contact-form spec)test:e2esequentially (portfoliothenblog) to avoid cross-app test server contention in monorepo runsValidation
pnpm test:e2emake preflightSummary by CodeRabbit
Chores
Refactor
Style