Skip to content

test(function): add back skipped syncV3 test and fix flaky test#18919

Merged
nshirley merged 1 commit intomainfrom
FXA-7108
May 27, 2025
Merged

test(function): add back skipped syncV3 test and fix flaky test#18919
nshirley merged 1 commit intomainfrom
FXA-7108

Conversation

@nshirley
Copy link
Contributor

Because

  • We had a skipped and flaky syncV3 test

This pull request

  • Removes the skip on blocked with an registered email, unregistered uid
  • Fixes a race condition in Non-Sync - no user signed into browser, no user signed in locally which has been flaking in CI.

Issue that this pull request solves

Closes: (FXA-7108)

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@nshirley nshirley marked this pull request as ready for review May 27, 2025 15:40
@nshirley nshirley requested a review from a team as a code owner May 27, 2025 15:40
const expectedCommand = webChannelMessage.message.command;
const response = webChannelMessage.message.data;

await this.page.evaluate(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The evaluate will run immediately but also means the page needs to be loaded (and can't be loaded after evaluation). By switching to addInitScript it's injected and run before any other scripts.

The flaky test problem that was happening is that the browser would navigate to the signin page, and already respond to the web channel message before playwright had a chance to intercept. This could be replicated locally by setting a slowmo value ~ 500ms, or by just placing a breakpoint on the respondToWebChannelMessage after the page navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked other tests that use this too and they still pass locally without modification, but if we see flakes in CI, then we can adjust when respondToWebChannelMessage is called in those tests too, similar to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and this is our most recent and common flaky test according to circle on nightly builds!

target,
testAccountTracker,
}) => {
test.skip(true, 'FXA-9868');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this since the linked ticket is done and the test passes locally now! 👍

@nshirley
Copy link
Contributor Author

nshirley commented May 27, 2025

Hm, the sync test I tried to fix is still messy - I have to leave this in ready for review for CI to run the tests, but it's not quite ready. I'll update this comment when it is ready for review

Looks like it's resolved!

Because:
* We had a skipped and flaky syncV3 test

This commit:
* Removes the skip on `blocked with an registered email, unregistered uid`
* Fixes a race condition in `Non-Sync - no user signed into browser, no user signed in locally` which has been flaking in CI.
Copy link
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Nice finds!

@nshirley nshirley merged commit 076c489 into main May 27, 2025
19 checks passed
@nshirley nshirley deleted the FXA-7108 branch May 27, 2025 18:58
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