[E2E] Stabilize a flaky Price Filter test #44690

WunderBart · 2024-02-16T10:30:18Z

Changes proposed in this Pull Request:

Stabilize a flaky Price Filter test by addressing the unstable nature of the filter input (see inline comments).

How to test the changes in this Pull Request:

The test should pass in CI. To test locally:

cd plugins/woocommerce-blocks
pnpm env:start
pnpm test:e2e:side-effects -g "should show only products that match the filter"

WunderBart · 2024-02-16T10:34:01Z

@lanej0, I've just noticed you recently updated that test as well in #44440. Let me know if this solution makes sense to you. It seems to be addressing the direct cause of the flakiness.

Also, would it make sense to report the weird filter behavior since the All Products block is going to be deprecated? cc: @gigitux

WunderBart · 2024-02-16T10:50:02Z

...oocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

-		await frontendUtils.selectTextInput( maxPriceInput );
-		await maxPriceInput.fill( '$10', {
-			// eslint-disable-next-line playwright/no-force-option
-			force: true,
-		} );
+		await maxPriceInput.dblclick();
+		await maxPriceInput.fill( '$10' );


Double click seems to be working just as well and is likely closer to how a user would select the value. 😄

github-actions · 2024-02-16T10:50:39Z

Hi @lanej0, @gigitux, @dinhtungdu, @kmanijak, @woocommerce/woo-fse

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

WunderBart · 2024-02-16T10:51:13Z

...oocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

-		await page.waitForResponse( ( response ) =>
-			response.url().includes( blockData.endpointAPI )
-		);


It's generally a bad practice to call and await waitForResponse/Request simultaneously since the response/request might already be over when this is called.

The Waiting for event guide section has some good examples of using that API correctly.

Also, this is not necessary for the test to succeed so I removed it 😄

WunderBart · 2024-02-16T10:52:17Z

...oocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

-		expect( products ).toHaveLength( 1 );
+		await expect( allProducts ).toHaveCount( 1 );


For locators, we can use the dedicated toHaveCount assertion.

WunderBart · 2024-02-16T11:44:55Z

...oocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

-			force: true,
-		} );
+		await maxPriceInput.dblclick();
+		await maxPriceInput.fill( '$10' );
 		await maxPriceInput.press( 'Tab' );


This feels weird, not from the test but from the feature implementation perspective. Hitting Tab changes the focus to another element and loses the filter from the sight, so is that how it's intended to work? Why not update on value change or Enter? Not sure who to mention here, actually 😛 cc: @gigitux

github-actions · 2024-02-16T12:22:46Z

Test Results Summary

Commit SHA: 33e4245

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 36s
E2E Tests	306	0	0	19	0	325	7m 13s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

This is usually a bad practice as we're allowing the tests to start before the DOM is loaded, which can occasionally time-out events like locators waiting for their respective elements to appear.

dinhtungdu

Besides the comment about timeout, this is looking good to me. Thank you for working on this!

dinhtungdu · 2024-02-20T11:23:50Z

...oocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

+		await page
+			.getByRole( 'textbox', {
+				name: 'Filter products by maximum price',
+				disabled: true,
+			} )
+			.waitFor( { timeout: 3000 } )
+			.catch( () => {
+				// Do not throw in case Playwright doesn't make it in time for the
+				// initial (pre-request) render.
+			} );


So, initially, we had flaky tests because we got the price input field, but it became disabled at the time we tried to change the price.

With this code, we try to get a disabled input first, for three seconds. Then we wait and get the active one.

But if there is an issue with the test environment, after three seconds, the input becomes disabled again, will we get the original flakiness again?

So, initially, we had flaky tests because we got the price input field, but it became disabled at the time we tried to change the price.

Not exactly. I'll try to explain using some example scenarios:

Failure Example Due to Race Condition:

Initialization: The price filter input field is rendered on the UI.

Interaction: The automation framework Playwright identifies the input field as enabled and inputs a value of $10.

Concurrent Processing: Concurrently, the application begins to fetch filter data, during which it programmatically disables the input field.

Data Integration: Upon completion of data retrieval, the application logic resets the price filter to a value of $90 based on the fetched data and re-enables the input field.

Assertion Failure: The test assertion fails as the expected value of $10 is overwritten by the reset operation, resulting in a value of $90.

Success Example Despite Race Condition:

Initialization: The price filter input field is rendered on the UI.

Data Pre-fetching: The application initiates the data fetching process for filter criteria and disables the input field.

Deferred Interaction: Playwright attempts to input a value of $10 but finds the input field disabled. It enters a wait state until the field becomes interactive.

Data Integration and Reactivation: Once data fetching concludes, the application resets the price filter value based on the retrieved data, defaulting it to $90, and subsequently re-enables the input field.

Successful Interaction: Playwright, detecting the re-enabled state of the input field, proceeds to set the filter value to $10.

Assertion Success: The test passes as Playwright successfully sets the expected value post-data fetching process, aligning with the test conditions.

But if there is an issue with the test environment, after three seconds, the input becomes disabled again, will we get the original flakiness again?

Fortunately, a disabled input is not an issue for Playwright as it will enter a waiting state until the input is enabled.

I still have another question 😄 .
Just theoretically, if there is an issue that delays the data fetching to more than three seconds after load:

Initialization: The price filter input field is rendered on the UI.

Delayed data fetching: the input field is still enabled.

Timed-out waiting for disabled input: PW keeps waiting for a disabled input and timed out after three seconds.

Interaction: After three seconds, The automation framework Playwright identifies the input field as enabled and inputs a value of $10.

Concurrent Processing: Concurrently, the application begins to fetch filter data, during which it programmatically disables the input field.

Data Integration: Upon completion of data retrieval, the application logic resets the price filter to a value of $90 based on the fetched data and re-enables the input field.

Assertion Failure: The test assertion fails as the expected value of $10 is overwritten by the reset operation, resulting in a value of $90.

Is this a valid assumption?

I can see this may not even be possible and your updates here will solve most of the flakiness we have. So please consider my comment here is just for learning and curiosity. The code change in this PR is LGTM.

Is this a valid assumption?

Playwright will time out and fail the test if the data fetching is delayed to over 3 seconds since the filter input has become visible in the DOM (which is when Playwright starts the disabled locator timer). Therefore, nothing after step no. 3 from the sequence you provided would happen.

I can see this may not even be possible and your updates here will solve most of the flakiness we have.

I do think that this is an edge-case scenario at most! 😄 While my fix addresses the flakiness, the actual fix should be provided in the app logic instead. The input should be rendered disabled, and only enabled once the data is initialized. That way we wouldn't need any extra handling in the test. Does that make sense?

So please consider my comment here is just for learning and curiosity. The code change in this PR is LGTM.

Thanks for diving into this PR and sharing your thoughts! 😊 I totally get where you're coming from with your questions. Glad to hear the changes look good to you. Happy to answer any more questions or ideas.

Thanks for your reply! It's clear now

dinhtungdu

As discussed in the review, this is LGTM!

Stabilize flaky price filter E2E test

2b37eb2

WunderBart requested review from lanej0, gigitux and dinhtungdu February 16, 2024 10:30

WunderBart added 2 commits February 16, 2024 11:37

Tweak comment

9fa14c0

Add changelog entry

286ee4f

woocommercebot requested a review from a team February 16, 2024 10:49

WunderBart commented Feb 16, 2024

View reviewed changes

WunderBart added 2 commits February 16, 2024 14:13

Don't start the tests after commit as the DOM might not be ready

233683c

This is usually a bad practice as we're allowing the tests to start before the DOM is loaded, which can occasionally time-out events like locators waiting for their respective elements to appear.

Fix the catching expression to prevent throwing

33e4245

WunderBart added the focus: e2e tests Issues related to e2e tests label Feb 16, 2024

WunderBart self-assigned this Feb 16, 2024

github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. and removed focus: e2e tests Issues related to e2e tests labels Feb 16, 2024

WunderBart requested a review from kmanijak February 19, 2024 12:20

dinhtungdu reviewed Feb 20, 2024

View reviewed changes

dinhtungdu approved these changes Feb 20, 2024

View reviewed changes

WunderBart merged commit 82d1df5 into trunk Feb 20, 2024
67 checks passed

WunderBart deleted the tweak/stabilize-flaky-price-filter-e2e branch February 20, 2024 15:30

github-actions bot added this to the 8.7.0 milestone Feb 20, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 20, 2024

alvarothomas added status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[E2E] Stabilize a flaky Price Filter test #44690

[E2E] Stabilize a flaky Price Filter test #44690

WunderBart commented Feb 16, 2024 •

edited

WunderBart commented Feb 16, 2024 •

edited

WunderBart Feb 16, 2024

github-actions bot commented Feb 16, 2024 •

edited

WunderBart Feb 16, 2024 •

edited

WunderBart Feb 16, 2024 •

edited

WunderBart Feb 16, 2024

WunderBart Feb 16, 2024

github-actions bot commented Feb 16, 2024 •

edited

dinhtungdu left a comment

dinhtungdu Feb 20, 2024

WunderBart Feb 20, 2024 •

edited

dinhtungdu Feb 20, 2024 •

edited

WunderBart Feb 20, 2024 •

edited

dinhtungdu Feb 20, 2024

dinhtungdu left a comment

		expect( products ).toHaveLength( 1 );
		await expect( allProducts ).toHaveCount( 1 );

[E2E] Stabilize a flaky Price Filter test #44690

[E2E] Stabilize a flaky Price Filter test #44690

Conversation

WunderBart commented Feb 16, 2024 • edited

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

WunderBart commented Feb 16, 2024 • edited

WunderBart Feb 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024 • edited

WunderBart Feb 16, 2024 • edited

Choose a reason for hiding this comment

WunderBart Feb 16, 2024 • edited

Choose a reason for hiding this comment

WunderBart Feb 16, 2024

Choose a reason for hiding this comment

WunderBart Feb 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024 • edited

Test Results Summary

dinhtungdu left a comment

Choose a reason for hiding this comment

dinhtungdu Feb 20, 2024

Choose a reason for hiding this comment

WunderBart Feb 20, 2024 • edited

Choose a reason for hiding this comment

dinhtungdu Feb 20, 2024 • edited

Choose a reason for hiding this comment

WunderBart Feb 20, 2024 • edited

Choose a reason for hiding this comment

dinhtungdu Feb 20, 2024

Choose a reason for hiding this comment

dinhtungdu left a comment

Choose a reason for hiding this comment

WunderBart commented Feb 16, 2024 •

edited

WunderBart commented Feb 16, 2024 •

edited

github-actions bot commented Feb 16, 2024 •

edited

WunderBart Feb 16, 2024 •

edited

WunderBart Feb 16, 2024 •

edited

github-actions bot commented Feb 16, 2024 •

edited

WunderBart Feb 20, 2024 •

edited

dinhtungdu Feb 20, 2024 •

edited

WunderBart Feb 20, 2024 •

edited