feat(prompt): link to error prompt in terminal #35341
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new feature to display an error prompt path in terminal outputs to help diagnose test failures. The changes include:
- Adding a new test in the reporter-line specs to validate the error prompt output.
- Enhancing the error formatting in the base reporter to append a relative path to error prompts when available.
- Updating the prompt handling and the HTML reporter to accommodate the new prompt attachment structure.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/playwright-test/reporter-line.spec.ts | Added a test case to check that the terminal output includes the error prompt path. |
| packages/playwright/src/reporters/base.ts | Modified error formatting functions to allow inclusion of the error prompt. |
| packages/playwright/src/prompt.ts | Updated to use fs/promises and write prompt attachments with a file path. |
| packages/html-reporter/src/testErrorView.tsx | Changed the prompt property type to TestAttachment and updated prompt handling accordingly. |
| packages/html-reporter/src/testResultView.tsx | Adjusted attachment retrieval to align with the new prompt attachment structure. |
| packages/playwright/src/reporters/github.ts | Updated reporter call to use the new error formatting signature. |
| packages/playwright/src/reporters/html.ts | Updated the call to formatResultFailure to include new parameters. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| if (includePrompt) { | ||
| const promptPath = result.attachments.find(a => a.name === `_prompt-${index}`)?.path; | ||
| if (promptPath) | ||
| message += '\n' + screen.colors.cyan(`Error Prompt: ${relativeFilePath(screen, config, promptPath)}`); |
There was a problem hiding this comment.
I think this is a layering violation. Test runner should not know about prompts.
There was a problem hiding this comment.
This isn't the test runner, but the terminal reporter. If the HTML reporter knows about prompts, why can't the terminal reporter?
packages/playwright/src/prompt.ts
Outdated
| ); | ||
| } | ||
|
|
||
| const promptPath = testInfo.outputPath(`_prompt-${index}.md`); |
There was a problem hiding this comment.
The file name does not have to start with the underscore 😄
There was a problem hiding this comment.
good point! and even better, it doesn't need to have an index if there's only one. fixed in d1cc9e9.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
packages/playwright/src/prompt.ts
Outdated
| if (!error.message) | ||
| return; | ||
| return false; | ||
| if (testInfo.attachments.find(a => a.name === `_prompt-${index}`)) |
There was a problem hiding this comment.
I think this filter condition conflicts with checking for errors.length === 1 below.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"2 flaky38921 passed, 805 skipped Merge workflow run. |
2 participants
Closes #35099 by adding a path to the prompt contents to terminal report outputs: