-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[37, 119, 191, 207, 105] Fix flaky test and remove retries #5142
Conversation
🦋 Changeset detectedLatest commit: a2b2ab4 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 |
fa121df
to
1d06a7d
Compare
@@ -18,45 +18,39 @@ test.beforeEach(({ page }) => { | |||
|
|||
const PRE_INSTALLATION_TIMEOUT = 20 * 1000; | |||
const INSTALLATION_PENDING_TIMEOUT = 50 * 1000; | |||
const APP_LISTED_TIMEOUT = 15 * 1000; | |||
const APP_INSTALLED_TIMEOUT = 50 * 1000; | |||
const APP_EXPECT_UI_TIMEOUT = 15 * 1000; |
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.
Do we need such long timeouts?
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.
const DEFAULT_WORKERS = "2"; | ||
// const DEFAULT_RETRIES = "1"; |
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.
do you need that comment?
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 want to see how disabling retries behaves and if needed revert this change.
@@ -257,6 +257,10 @@ test("TC: SALEOR_84 Create draft order @e2e @draft", async () => { | |||
}); | |||
|
|||
test("TC: SALEOR_191 Refund products from the fully paid order @e2e @refunds", async () => { | |||
// All steps of this test pass (including after hooks), but Playwright | |||
// marks it as failed because of exceeding 30s timeout | |||
test.slow(); |
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 marks test as slow, and triples the timeout, meaning something similar as if you were added 3x base timeout here. I does not look like fix for the flaky test.
Since you are fixing couple of test in single PR, please mark this as TODO, and leave ticket number for further fixing.
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.
Last time it failed the test threw the following error:
Test finished within timeout of 30000ms, but tearing down "context" ran out of time.
Please allow more time for the test, since teardown is attributed towards the test timeout budget.
here's the recording:
32_timeout_PW.webm - every step passed
* SALEOR_37 fix * add remove retires comment * cleanup * expect items instead of waiting for DOM * retain traces on first failure * 119 and 191 * changeset * change global timeouts * improve 119 * remove action timeout * 105 fix * SALEOR_207 wait for currency dropdown to load * wait for currency to be visible * keep one number convention * change changeset
What type of PR is this?
Related Issues or Documents
Usage Instructions, Screenshots, Recordings
Have you written tests?
[Optional] Description