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.

Sign up for GitHub

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

Jump to bottom

[E2E] Stabilize a flaky Price Filter test #44690

Merged
merged 5 commits into from Feb 20, 2024
Merged

[E2E] Stabilize a flaky Price Filter test #44690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b37eb2
Stabilize flaky price filter E2E test
WunderBart Feb 16, 2024
9fa14c0
Tweak comment
WunderBart Feb 16, 2024
286ee4f
Add changelog entry
WunderBart Feb 16, 2024
233683c
Don't start the tests after `commit` as the DOM might not be ready
WunderBart Feb 16, 2024
33e4245
Fix the catching expression to prevent throwing
WunderBart Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 22 additions & 16 deletions ...ommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@
await editor.publishPost();
const url = new URL( page.url() );
const postId = url.searchParams.get( 'post' );
await page.goto( `/?p=${ postId }`, { waitUntil: 'commit' } );
await page.goto( `/?p=${ postId }` );
} );

test( 'should show all products', async ( { frontendUtils } ) => {
Expand All @@ -59,24 +59,30 @@
page,
frontendUtils,
} ) => {
// The price filter input is initially enabled, but it becomes disabled
// for the time it takes to fetch the data. To avoid setting the filter
// value before the input is properly initialized, we wait for the input
// to be disabled first. This is a safeguard to avoid flakiness which
// should be addressed in the code, but All Products block will be
// deprecated in the future, so we are not going to optimize it.
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.
} );
Comment on lines +68 to +77
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Initialization: The price filter input field is rendered on the UI.
  2. Interaction: The automation framework Playwright identifies the input field as enabled and inputs a value of $10.
  3. Concurrent Processing: Concurrently, the application begins to fetch filter data, during which it programmatically disables the input field.
  4. 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.
  5. 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:

  1. Initialization: The price filter input field is rendered on the UI.
  2. Data Pre-fetching: The application initiates the data fetching process for filter criteria and disables the input field.
  3. 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.
  4. 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.
  5. Successful Interaction: Playwright, detecting the re-enabled state of the input field, proceeds to set the filter value to $10.
  6. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. Initialization: The price filter input field is rendered on the UI.
  2. Delayed data fetching: the input field is still enabled.
  3. Timed-out waiting for disabled input: PW keeps waiting for a disabled input and timed out after three seconds.
  4. Interaction: After three seconds, The automation framework Playwright identifies the input field as enabled and inputs a value of $10.
  5. Concurrent Processing: Concurrently, the application begins to fetch filter data, during which it programmatically disables the input field.
  6. 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.
  7. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your reply! It's clear now


const maxPriceInput = page.getByRole( 'textbox', {
name: 'Filter products by maximum price',
} );

// All Products block will be deprecated in the future, so we are not going to optimize it.

// eslint-disable-next-line playwright/no-networkidle
await page.waitForLoadState( 'networkidle' );

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' );
Comment on lines -71 to +84
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

await maxPriceInput.press( 'Tab' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

await page.waitForResponse( ( response ) =>
response.url().includes( blockData.endpointAPI )
);
Comment on lines -77 to -79
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


const allProductsBlock = await frontendUtils.getBlockByName(
'woocommerce/all-products'
Expand All @@ -89,9 +95,9 @@
blockData.placeholderUrl
);

const products = await allProductsBlock.getByRole( 'listitem' ).all();
const allProducts = allProductsBlock.getByRole( 'listitem' );

expect( products ).toHaveLength( 1 );
await expect( allProducts ).toHaveCount( 1 );
Comment on lines -94 to +100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For locators, we can use the dedicated toHaveCount assertion.

expect( page.url() ).toContain(
blockData.urlSearchParamWhenFilterIsApplied
);
Expand Down Expand Up @@ -141,7 +147,7 @@
.locator( '.product' )
.all();

expect( products ).toHaveLength( 16 );

Check failure on line 150 in plugins/woocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright E2E tests - SideEffects

[blockThemeWithGlobalSideEffects] › price-filter/price-filter.block_theme.side_effects.spec.ts:138:6 › woocommerce/price-filter Block - with PHP classic template › should show all products 

  1) [blockThemeWithGlobalSideEffects] › price-filter/price-filter.block_theme.side_effects.spec.ts:138:6 › woocommerce/price-filter Block - with PHP classic template › should show all products 
    Error: expect(received).toHaveLength(expected)

    Expected length: 16
    Received length: 14
    Received array:  [{"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=0"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=1"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=2"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=3"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=4"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=5"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=6"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=7"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=8"}, {"_frame": {"_guid": "frame@81ab8a38b77f8e3e1fcedf215a397234", "_type": "Frame"}, "_selector": "[data-block-name=\"woocommerce/legacy-template\"] >> internal:role=list >> .product >> nth=9"}, …]

      148 | 			.all();
      149 |
    > 150 | 		expect( products ).toHaveLength( 16 );
          | 		                   ^
      151 | 	} );
      152 |
      153 | 	// eslint-disable-next-line playwright/no-skipped-test

        at /home/runner/work/woocommerce/woocommerce/plugins/woocommerce-blocks/tests/e2e/tests/price-filter/price-filter.block_theme.side_effects.spec.ts:150:22
} );

// eslint-disable-next-line playwright/no-skipped-test
Expand Down
4 changes: 4 additions & 0 deletions plugins/woocommerce/changelog/tweak-stabilize-flaky-price-filter-e2e
View file
Open in desktop
@@ -0,0 +1,4 @@
Significance: patch
Type: dev

Stabilize a flaky Price Filter test by addressing the unstable nature of the filter input.