-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Permafail E2E: fix Notifications spec. #78616
Conversation
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. |
- migrate commentingUser to the new schema.
- refactor the component POM to use modern meethods. packages/calypso-e2e/src/lib/test-account.ts - add new flag for `authenticate` call to allow waiting for the full load. test/e2e/specs/infrastructure/notification__actions.ts - refactor the spec to only perform one action, not to perform and undo the action.
- explain the purpose of the spec with JSDoc. - rename the file. packages/calypso-e2e/src/lib/test-account.ts - change optional parameter for `authenticate` to "waitUntiLStable".
98c58d8
to
1565b3d
Compare
@@ -23,6 +14,8 @@ export class NotificationsComponent { | |||
*/ | |||
constructor( page: Page ) { | |||
this.page = page; | |||
// There is no accessible locator for this panel. | |||
this.anchor = page.locator( 'div[id=wpnc-panel]' ); |
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 is something I've yet to codify in the E2E documentation but follows the precedent set in WordPress/gutenberg (example).
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 see what you meant. TIL about the full accessibility tree in Chrome DevTools!
async authenticate( page: Page, { url }: { url?: string | RegExp } = {} ): Promise< void > { | ||
async authenticate( | ||
page: Page, | ||
{ url, waitUntilStable }: { url?: string | RegExp; waitUntilStable?: boolean } = {} |
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 change and the accompanying changes in Line 57-60 are important for stable loading of specs that rely on interaction in the My Home page.
Because Calypso loads so slowly but Playwright executes at lightning speed, we can often end up in a situation where Calypso "swallows" interactions on the page. To work around this issue, we can wait until the page loads entirely, but on Simple sites, this is often in excess of 15-20 seconds.
However, the Sidebar component is rendered and ready to interact towards the middle-later portions of the loading process. Waiting for the Sidebar component to become ready is suitable 98% of the time.
|
||
it( 'Mark comment as spam', async function () { | ||
await notificationsComponent.clickNotificationAction( 'Spam' ); | ||
await notificationsComponent.clickUndo(); |
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.
Undo the action to mark the comment as spam so:
- we don't need to create another comment to perform the Trash action, and;
- we don't inadvertently train the backend systems to consider
commentingUser
as a spammer.
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.
LGTM 🚀
@@ -23,6 +14,8 @@ export class NotificationsComponent { | |||
*/ | |||
constructor( page: Page ) { | |||
this.page = page; | |||
// There is no accessible locator for this panel. | |||
this.anchor = page.locator( 'div[id=wpnc-panel]' ); |
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 see what you meant. TIL about the full accessibility tree in Chrome DevTools!
- use changes introduced in #78616.
* test/e2e/specs/support/support__popover.ts - remove spec in favor of a (yet-to-be-written) component test. test/e2e/specs/support/support__showme-where.ts - renamed. test/e2e/specs/help-center/help-center__general-interaction.ts - refactored to run through the support help docs, then the calypso link. packages/calypso-e2e/src/lib/components/support-component.ts - use accessible selectors. - remove methods no longer used. * Fix issues pointed out in review. packages/calypso-e2e/src/lib/components/support-component.ts - fix typo. test/e2e/specs/help-center/help-center__general-interaction.ts - select the second help doc instead of first. * test/e2e/specs/help-center/help-center__general-interaction.ts - use changes introduced in #78616. * test/e2e/specs/help-center/help-center__general-interaction.ts - add step to clear results. * packages/calypso-e2e/src/lib/components/support-component.ts - add workaround for mobile by checking whether the Close Search button is present.
Fixes #78103.
Proposed Changes
This PR fixes the cause of the permafailing Notifications spec and refactors the spec to modern standards.
Key changes:
NotificationComponent
to use modern accessible locators.commentingUser
entry in the encrypted secrets file to modern standards.waitUntilStable
in theTestAccount.authenticate
method.Testing Instructions
sEnsure the following build configurations are passing:
Pre-merge Checklist