Skip to content
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

Introduce a custom reporter for Playwright #1165

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iAmmar7
Copy link
Collaborator

@iAmmar7 iAmmar7 commented Jan 21, 2025

Description

This PR includes:

  1. A custom reporter for the Playwright end-to-end test for the Browser SDKs.
  2. Set maxFailures to 1 in the Playwright config so that the failed test is the last one on the logs which makes it easier to debug.
  3. Clean the context before moving to the next test.

Type of change

  • Internal refactoring
  • Bug fix (bugfix - non-breaking)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

In case of new feature or breaking changes, please include code snippets.

Copy link

changeset-bot bot commented Jan 21, 2025

⚠️ No Changeset found

Latest commit: 60379ad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@iAmmar7 iAmmar7 changed the title WIP: Introduce a custom reporter for Playwright Introduce a custom reporter for Playwright Jan 21, 2025
@iAmmar7
Copy link
Collaborator Author

iAmmar7 commented Jan 21, 2025

I have made this change to make it easier for me to debug. However, you guys can also read the logs in this PR's CI and see if it is helpful for you guys to debug and understand the test failure. If yes, we can consider merging this change. @ryanwi @giavac

Since we have a custom reporter now, I can also make any other improvements if required.

One improvement (or change) I was considering making is to log each message with the test title. Currently, we see logs starting with [page] or [pageOne], etc. I can change it to [test title]. This way, looking at each log, we would know from which test this log is coming. However, the test title can be a long text, and I am not sure if this will be more helpful or if it will pollute the logs.

Open to suggestions.

@iAmmar7
Copy link
Collaborator Author

iAmmar7 commented Jan 21, 2025

Re-running the CI just to make sure it's not showing false positive results.

@giavac
Copy link
Collaborator

giavac commented Jan 22, 2025

One improvement (or change) I was considering making is to log each message with the test title. Currently, we see logs starting with [page] or [pageOne], etc. I can change it to [test title]

I like this idea. Maybe each test could have both a title and a log_prefix defined. So the title could be very descriptive while the log prefix string could be shorter but still unique per test.

@iAmmar7
Copy link
Collaborator Author

iAmmar7 commented Jan 22, 2025

I like this idea. Maybe each test could have both a title and a log_prefix defined. So the title could be very descriptive while the log prefix string could be shorter but still unique per test.

I realized that the test title would still not be enough considering that a single test can create multiple pages, that's why we differentiate those pages with pageOne, pageTwo, etc.

So, your suggestion is better to uniquely identify each log based on the page and test. We can implement this log_prefix logic, but we do not need the custom reporter for that. We will just need to pass that log_prefix while creating a page for each test. For eg, here; https://github.com/signalwire/signalwire-js/blob/main/internal/e2e-js/tests/roomSession.spec.ts#L17

I will do that in this PR; I would have to add this small change in all the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants