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

fix: Test Snap Cronjob can trigger a cronjob to open a di... flaky tests #28363

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

hjetpoluru
Copy link
Contributor

@hjetpoluru hjetpoluru commented Nov 7, 2024

Description

This flaky test is due to the dialog taking time to appear and disappear, which is the expected behavior. I have attached a video demonstrating the current behavior.

Cronjob-snap.mov

To address this issue, I added retry logic and validation. However, I noticed that when the dialog does not appear, switchToWindowWithTitle method throws an exception and fails the test hence causing the retry logic to be ineffective. Implementation is in the first commit in this PR b97dbbe.

Fixing this issue within the framework would require significant effort and could impact other tests. Therefore, I have implemented a wait period before attempting to switch to the window. This approach ensures that the dialog has enough time to appear, thereby reducing the likelihood of test failures.

Open in GitHub Codespaces

Related issues

Fixes:
#28154

Manual testing steps

Run the test locally or in codespaces using below command
yarn
yarn build:test:webpack
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-cronjob.spec.js --browser=chrome --leave-running

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@hjetpoluru hjetpoluru added team-extension-platform Extension Platform team area-qa Relating to QA work (Quality Assurance) labels Nov 7, 2024
@hjetpoluru hjetpoluru self-assigned this Nov 7, 2024
@hjetpoluru hjetpoluru requested a review from a team as a code owner November 7, 2024 17:41
Copy link
Contributor

github-actions bot commented Nov 7, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@hjetpoluru hjetpoluru marked this pull request as draft November 7, 2024 17:41
@hjetpoluru hjetpoluru marked this pull request as ready for review November 8, 2024 14:55
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 8, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [5f92210]
Page Load Metrics (2244 ± 108 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31326812054619297
domContentLoaded186425662198222106
load187826682244224108
domInteractive1890502110
backgroundConnect993483014
firstReactRender813661557134
getState793282512
initialActions01000
loadScripts13631931164517986
setupStore85917157
uiStartup209031422569275132
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@@ -74,6 +75,8 @@ describe('Test Snap Cronjob', function () {
text: 'Reconnect to Cronjobs Snap',
});

await driver.delay(largeDelayMs);

// switch to dialog popup, wait for a maximum of 65 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the comment wait for a maximum of 65 seconds

It used to do that in a previous version, but it no longer does, and that's actually the problem here. It used to have
windowHandles = await driver.waitUntilXWindowHandles(3, 1000, 65000);

I researched it, and there's no easy way to increase the timeout in my new switchToWindowWithTitle (oops)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will remove the comment. Thanks for checking @HowardBraham.

Copy link
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

LGTM

@hjetpoluru hjetpoluru enabled auto-merge November 12, 2024 14:03
@metamaskbot
Copy link
Collaborator

Builds ready [7a33bab]
Page Load Metrics (2731 ± 682 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint475682622311388667
domContentLoaded1632670426991406675
load1636683327311420682
domInteractive2838610610148
backgroundConnect6194394120
firstReactRender4968321617182
getState6112283015
initialActions00000
loadScripts1193564520411153554
setupStore5119243014
uiStartup1822782331601660797
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@hjetpoluru hjetpoluru added this pull request to the merge queue Nov 12, 2024
Merged via the queue into develop with commit 81008f6 Nov 12, 2024
76 checks passed
@hjetpoluru hjetpoluru deleted the cronjob-snap-flaky-test-28154-fix branch November 12, 2024 14:48
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-qa Relating to QA work (Quality Assurance) INVALID-PR-TEMPLATE PR's body doesn't match template release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-extension-platform Extension Platform team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants