feat(e2e): add screenshots for failing tests with vitest runner#1597
feat(e2e): add screenshots for failing tests with vitest runner#1597
Conversation
commit: |
OrbisK
force-pushed
the
feat/vitest-failing-screenshot
branch
from
8542a03 to
310349c
Compare
|
cc @danielroe I am not sure how to add tests to check if this is working. But I have tested the prerelease. I am not sure if we need an opt-out option. And if the generated image path is good enough 😄 |
📝 WalkthroughWalkthroughThe pull request modifies Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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)
src/e2e/browser.ts (1)
59-61: Avoid a fully silent catch for screenshot failures.Line 59 swallows all failures, which makes debugging missing screenshots hard. Consider logging under a debug flag.
Suggested tweak
- catch { - // noop + catch (error) { + if (process.env.NUXT_TEST_DEBUG) { + console.warn('[nuxt/test-utils] failed to capture failure screenshot:', error) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/browser.ts` around lines 59 - 61, The empty catch in src/e2e/browser.ts is swallowing screenshot errors; update the catch to log the error and context instead of being fully silent — e.g., inside the try/catch around the screenshot call (the block currently written as catch { // noop }) call a debug-level logger (or console.debug) conditioned on a debug flag (process.env.DEBUG or an existing isDebug flag), include the error message and identifying info (page/url/filename), and keep the retry/no-throw behavior so tests don't fail but missing screenshots are traceable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/e2e/browser.ts`:
- Line 56: Sanitize test.task.fullTestName before interpolating into the
screenshot filename: update the expression used in the join(...) call (the line
constructing path with `test.task.fullTestName`) to replace or remove
filesystem-invalid characters (e.g., <>:"/\\|?* and control chars) or use a
small helper like `sanitizeFilename(name)`/`slugify` to produce a safe string,
then use that sanitized value in the template `failed-${sanitizedName}.png`;
ensure the helper is imported/defined near the top and used where
`test.task.fullTestName` is currently referenced so screenshots never fail due
to invalid filename characters.
---
Nitpick comments:
In `@src/e2e/browser.ts`:
- Around line 59-61: The empty catch in src/e2e/browser.ts is swallowing
screenshot errors; update the catch to log the error and context instead of
being fully silent — e.g., inside the try/catch around the screenshot call (the
block currently written as catch { // noop }) call a debug-level logger (or
console.debug) conditioned on a debug flag (process.env.DEBUG or an existing
isDebug flag), include the error message and identifying info
(page/url/filename), and keep the retry/no-throw behavior so tests don't fail
but missing screenshots are traceable.
| vitest.onTestFailed(async (test) => { | ||
| try { | ||
| await page.screenshot({ | ||
| path: join(dirname(test.task.file.filepath), '__screenshots__', `failed-${test.task.fullTestName}.png`), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific line in question
cat -n src/e2e/browser.ts | head -70 | tail -20Repository: nuxt/test-utils
Length of output: 799
🏁 Script executed:
# Run the verification commands from the review
rg -nP --type=ts -C3 'fullTestName.*\.png|__screenshots__' srcRepository: nuxt/test-utils
Length of output: 446
🏁 Script executed:
# Check for any existing sanitization functions
rg -nP --type=ts -C2 'sanitize|slug|replace\(.+[<>:"/\\|?*]' src/e2eRepository: nuxt/test-utils
Length of output: 41
🏁 Script executed:
# Check if there are other places where test names are used as filenames
rg -nP --type=ts 'test\.task\.' src/e2e | grep -E '\.(png|jpg|jpeg|txt|json)'Repository: nuxt/test-utils
Length of output: 195
🏁 Script executed:
# Verify that test names can indeed contain problematic characters
# by checking if there are any test name patterns in config or other files
rg -nP --type=ts 'fullTestName' srcRepository: nuxt/test-utils
Length of output: 195
🏁 Script executed:
# Let's verify what characters are actually allowed in filenames across platforms
# and understand the severity of this issue by checking if there are any existing test names
# that might trigger this problem
rg -nP --type=ts 'describe\(|it\(' src/e2e | head -20Repository: nuxt/test-utils
Length of output: 261
🏁 Script executed:
# Let's look at actual test files to see what test names might look like
find src -name "*.test.ts" -o -name "*.spec.ts" | head -5Repository: nuxt/test-utils
Length of output: 41
🏁 Script executed:
# Also check if there are any other usages of screenshot or similar file operations
rg -nP --type=ts 'screenshot|__screenshots__' srcRepository: nuxt/test-utils
Length of output: 249
Sanitize fullTestName before using it as a filename.
Line 56 builds the filename from test.task.fullTestName directly. Test names commonly include characters that are invalid on some filesystems (< > : " / \ | ? *), so screenshot capture can silently fail on those platforms.
💡 Proposed fix
+function sanitizeScreenshotName(name: string): string {
+ return name
+ .replace(/[<>:"/\\|?*\u0000-\u001F]/g, '_')
+ .replace(/\s+/g, ' ')
+ .trim()
+ .slice(0, 180)
+}
+
export async function createPage(path?: string, options?: BrowserContextOptions): Promise<NuxtPage> {
const ctx = useTestContext()
const browser = await getBrowser()
const page = await browser.newPage(options) as unknown as NuxtPage
@@
vitest.onTestFailed(async (test) => {
try {
+ const fileName = `failed-${sanitizeScreenshotName(test.task.fullTestName)}.png`
await page.screenshot({
- path: join(dirname(test.task.file.filepath), '__screenshots__', `failed-${test.task.fullTestName}.png`),
+ path: join(dirname(test.task.file.filepath), '__screenshots__', fileName),
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e/browser.ts` at line 56, Sanitize test.task.fullTestName before
interpolating into the screenshot filename: update the expression used in the
join(...) call (the line constructing path with `test.task.fullTestName`) to
replace or remove filesystem-invalid characters (e.g., <>:"/\\|?* and control
chars) or use a small helper like `sanitizeFilename(name)`/`slugify` to produce
a safe string, then use that sanitized value in the template
`failed-${sanitizedName}.png`; ensure the helper is imported/defined near the
top and used where `test.task.fullTestName` is currently referenced so
screenshots never fail due to invalid filename characters.
danielroe
changed the title
1 participant
🔗 Linked issue
❓ Type of change
📚 Description
This PR adds a screenshot if a e2e browser tests fails.
Todos:
explore vitest artifacts