E2E: refactor Support specs into one flow. #76960

worldomonation · 2023-05-16T02:17:04Z

Proposed Changes

This PR is a semi-major refactor of how we think about Support related E2E specs. These specs were a near carbon copy of the ones that existed back in the Selenium framework days (prior to 2021).

Now that we have the ability to execute component testing with `react-testing-library, most of what's covered in the specs (even in revised form) should be tested at the component level.

However, to address the issue of permanent failure in #76916, I've decided to restructure the specs as a temporary bridge until they are moved into the component testing level.

Key changes:

merge all scenarios (invalid, valid) into one.
discontinue testing the support card present in /home.
discontinue parametrization of the overall suite, instead using the environment variables to run on the targeted environment.

Testing Instructions

Ensure the following build configurations are passing:

Calypso E2E (desktop)
Calypso E2E (mobile)
Pre-Release E2E

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

github-actions · 2023-05-16T02:17:22Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-67039

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-67039&env=jetpack

matticbot · 2023-05-16T03:01:24Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

worldomonation · 2023-05-16T04:12:39Z

packages/calypso-e2e/src/lib/components/support-component.ts

-			this.page.waitForSelector( selectors.resultsPlaceholderHelpCenter, { state: 'hidden' } ),
-			this.page.waitForSelector( selectors.resultsPlaceholder, { state: 'hidden' } ),
-		] );
+		this.root = this.page.getByLabel( 'Help Center' );


This is a technique/style I learned from working on the Gutenberg Playwright migration, specifically this comment.

The idea is that, when using role-based selectors (as recommended by Playwright now), we trade legibility for some ambiguity in the locator structure. For instance, the following locator is easy to read and understand, but there is potential to match on multiple elements on a page.

page.getByRole('button', { name: 'home' });

By always selecting from a parent/root locator it is possible to understand the overall structure hierarchy.

Note: if I was following the Gutenberg code style, this would probably be named something like this.helpCenterinstead of this.root.

worldomonation · 2023-05-16T05:53:53Z

test/e2e/specs/support/support__popover.ts

+			expect( articles ).toBeTruthy();
+			expect( links ).toBeTruthy();


This change in assertion is also from my experience with Gutenberg Playwright migration. In working on the PR, one thing that was repeated over the course of the PR is to not structure tests around implementation details.

In the case of Support testing, my thoughts are that the following constitute implementation detail:

how the results are presented;

the order in which the results are presented.

Additionally, we cannot control:

names of the returned resources;

how many results are returned.

Throw on top of this the fact that the reworked Support popover does not have a true "empty results" state anymore and it becomes apparent the existing component is no longer fit for purpose.

By removing the assertion for default state is shown or empty results are shown and instead testing that the query returned some sort of results for the expected categories, implementation detail testing is removed as much as possible.

In this case, I'd go for the isGreaterThan( 0 ) matcher instead of toBeTruthy(). The reason for that is that articles or links could be an array as well, and expect( [] ).toBeTruthy() would return true. Being a little bit more explicit here could potentially guard us against some false negatives in the future.

The reason for that is that articles or links could be an array as well, and expect( [] ).toBeTruthy() would return true.

Great point and something I have yet to come to grips with despite a couple of years of working in JS/TS ecosystem.

Assertions changed in 23ed24e76e93e4ec1b0598cbedc0e6ad2f89f4dd, but may change again if the loop stress test fails.

worldomonation · 2023-05-16T06:14:04Z

test/e2e/specs/support/support__home.ts

This spec has been removed because of a couple of reasons:

the support card present in /home serves up an older version of the inline help.

usage for the support card in /home is lower than the popover help.

For the second point, I compared the Tracks events for the two different Supports that are in production.

calypso_inlinehelp_show (new Support)

calypso_inlinehelp_article_open (legacy Support)

For the legacy Support, the calypso_inlinehelp_article_open stands in for the number of times a user interacts with the card and opens a link. It is not the perfect approximation, but there is no Tracks event exclusively for the user interacting with the card.

For a 7-day trailing period as of today, the popover has ~66k interactions while the card has just under half at ~31k interactions. So while not trivial, the card variant sees significantly fewer interactions.

worldomonation · 2023-05-16T06:41:49Z

Looks like the current solution failed in a loop at 69 (nice) iterations.

WunderBart

Just a few non-blocking comments. Thanks! 🚀

WunderBart · 2023-05-16T10:15:09Z

packages/calypso-e2e/src/lib/components/support-component.ts

 		await Promise.all( [
-			// Waits for all placeholder CSS elements to be removed from the DOM.
-			this.waitForQueryComplete(),
 			this.page.waitForSelector( selectors.supportPopoverButton() ),
 			this.page.click( selectors.supportPopoverButton() ),
 		] );


I'm not sure I understand this chain - why do we need to wait for the same selector we're about to click here? I think it's what internally happens with click() - before it clicks, it waits for the given selector to be clickable, so the click itself should be sufficient.

I think this was a mistake on my part. Can't think of reasons to explain this, all I can maybe come up with is that I mistyped a selector and forgot about it.

Edit: it looks like this was in place in the original file. Even more of a head scratcher.

packages/calypso-e2e/src/lib/components/support-component.ts

WunderBart · 2023-05-16T10:53:16Z

test/e2e/specs/support/support__popover.ts

+			expect( articles ).toBeTruthy();
+			expect( links ).toBeTruthy();


In this case, I'd go for the isGreaterThan( 0 ) matcher instead of toBeTruthy(). The reason for that is that articles or links could be an array as well, and expect( [] ).toBeTruthy() would return true. Being a little bit more explicit here could potentially guard us against some false negatives in the future.

test/e2e/specs/support/support__showme-where.ts

major refactor - use locators and role-based locators where possible - replace single-purpose methods with multi-purpose methods

test/e2e/specs/support/support__home.ts - discontinue. test/e2e/specs/support/support__showme-where.ts - refactor to use the popover. test/e2e/specs/support/support__popover-invalid.ts - merge into one. test/e2e/specs/support/support__popover-valid.ts - merge into one. test/e2e/specs/support/support__popover.ts - merged spec that runs the "invalid" and "valid" choices. - discontinue parametrization.

- rename step packages/calypso-e2e/src/lib/components/support-component.ts - add wait for the placeholder to disappear

- replace the placeholder CSS

- replace `this.root` with `this.helpCenter`.

- return named values instead of array of values - add comment for the URL being matched. test/e2e/specs/support/support__popover.ts - change how the values being returned are referenced. - change the assertions to check `greaterthan` and `tobe`. test/e2e/specs/support/support__showme-where.ts - add URL example

- refactor the popover methods

- wait for the results to populate first

- remove debug message

- remove commented out code - update comment

worldomonation · 2023-05-17T06:49:40Z

packages/calypso-e2e/src/lib/components/support-component.ts

-			this.page.waitForSelector( selectors.resultsPlaceholderHelpCenter, { state: 'hidden' } ),
-			this.page.waitForSelector( selectors.resultsPlaceholder, { state: 'hidden' } ),
-		] );
+		this.helpCenter = this.page.locator( '.components-card__body > .inline-help__search' );


Changed: locator now points to the results field (in red) not the entire Inline Help modal.

This is because there is no selector to select the entire modal that is visible.

worldomonation · 2023-05-17T08:21:14Z

The new solution wasn't perfect (failed at 94 iterations) but it was much better than the previous ones I had come up with, plus I didn't want to block #76734.

worldomonation changed the title ~~E2E:~~ E2E: refactor Support specs into one flow. May 16, 2023

worldomonation self-assigned this May 16, 2023

worldomonation marked this pull request as ready for review May 16, 2023 03:56

worldomonation requested a review from a team as a code owner May 16, 2023 03:56

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 16, 2023

worldomonation commented May 16, 2023

View reviewed changes

worldomonation requested a review from a team May 16, 2023 06:16

WunderBart approved these changes May 16, 2023

View reviewed changes

worldomonation added 8 commits May 17, 2023 15:41

packages/calypso-e2e/src/lib/components/support-component.ts

39fb5e9

major refactor - use locators and role-based locators where possible - replace single-purpose methods with multi-purpose methods

test/e2e/specs/support/support__popover.ts

ba5c55a

- rename step packages/calypso-e2e/src/lib/components/support-component.ts - add wait for the placeholder to disappear

packages/calypso-e2e/src/lib/components/support-component.ts

d5a6b2a

- replace the placeholder CSS

packages/calypso-e2e/src/lib/components/support-component.ts

98b5b25

- replace `this.root` with `this.helpCenter`.

packages/calypso-e2e/src/lib/components/support-component.ts

0f707be

- refactor the popover methods

packages/calypso-e2e/src/lib/components/support-component.ts

3c656b6

- wait for the results to populate first

worldomonation force-pushed the fix/e2e-support-spec-76916 branch from f53cfb7 to 3c656b6 Compare May 17, 2023 06:41

worldomonation added 2 commits May 17, 2023 15:41

packages/calypso-e2e/src/lib/components/support-com

0b26313

- remove debug message

packages/calypso-e2e/src/lib/components/support-component.ts

0f98b40

- remove commented out code - update comment

worldomonation commented May 17, 2023

View reviewed changes

worldomonation merged commit aa68334 into trunk May 17, 2023

worldomonation deleted the fix/e2e-support-spec-76916 branch May 17, 2023 07:54

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 17, 2023

worldomonation mentioned this pull request May 17, 2023

Fetch all support articles from the same endpoint #76734

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E: refactor Support specs into one flow. #76960

E2E: refactor Support specs into one flow. #76960

worldomonation commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited by matticbot

Loading

matticbot commented May 16, 2023

worldomonation May 16, 2023

worldomonation May 16, 2023

WunderBart May 16, 2023 •

edited

Loading

worldomonation May 17, 2023

worldomonation May 16, 2023 •

edited

Loading

worldomonation commented May 16, 2023 •

edited

Loading

WunderBart left a comment

WunderBart May 16, 2023

worldomonation May 17, 2023 •

edited

Loading

WunderBart May 16, 2023 •

edited

Loading

worldomonation May 17, 2023

worldomonation commented May 17, 2023

		expect( articles ).toBeTruthy();
		expect( links ).toBeTruthy();

E2E: refactor Support specs into one flow. #76960

E2E: refactor Support specs into one flow. #76960

Conversation

worldomonation commented May 16, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented May 16, 2023 • edited by matticbot Loading

matticbot commented May 16, 2023

worldomonation May 16, 2023

Choose a reason for hiding this comment

worldomonation May 16, 2023

Choose a reason for hiding this comment

WunderBart May 16, 2023 • edited Loading

Choose a reason for hiding this comment

worldomonation May 17, 2023

Choose a reason for hiding this comment

worldomonation May 16, 2023 • edited Loading

Choose a reason for hiding this comment

worldomonation commented May 16, 2023 • edited Loading

WunderBart left a comment

Choose a reason for hiding this comment

WunderBart May 16, 2023

Choose a reason for hiding this comment

worldomonation May 17, 2023 • edited Loading

Choose a reason for hiding this comment

WunderBart May 16, 2023 • edited Loading

Choose a reason for hiding this comment

worldomonation May 17, 2023

Choose a reason for hiding this comment

worldomonation commented May 17, 2023

worldomonation commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited by matticbot

Loading

WunderBart May 16, 2023 •

edited

Loading

worldomonation May 16, 2023 •

edited

Loading

worldomonation commented May 16, 2023 •

edited

Loading

worldomonation May 17, 2023 •

edited

Loading

WunderBart May 16, 2023 •

edited

Loading