-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a trailing log line in case another playwright reporter removes our last line #613
Conversation
🦋 Changeset detectedLatest commit: 23ff98e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -403,6 +403,8 @@ export default class ReplayPlaywrightReporter implements Reporter { | |||
console.warn(`[replay.io]: ${line}`); | |||
}); | |||
} | |||
|
|||
console.log("[replay.io]:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really odd on its own. Maybe we could add some text to this, like this?
console.log("[replay.io]:"); | |
console.log("[replay.io]: Completed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it looks odd since we already output some (almost) empty lines like this. I see this line as padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This padding always bothered me - I intuitively expected to find some extra text below it. I think Brian had similar sentiment since he removed this recently here.
Perhaps it's the fact that this line is not completely empty (it has our prefix) makes is weird to me 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if this were just be an empty line :) but it's at least better this way than for Playwright to trim it off.
packages/playwright/src/reporter.ts
Outdated
// | ||
// reporters: [replayReporter({ upload: true }), ['line']] | ||
// | ||
// that can lead to removing *our* last log line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// that can lead to removing *our* last log line | |
// that can lead to removing *our* last log line | |
// | |
// the issue is tracked here: https://github.com/microsoft/playwright/issues/23875 |
Co-authored-by: Mateusz Burzyński <[email protected]>
No description provided.