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.

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

E2E test: Only expand the post summary section if it exists since the summary section will no longer be expansible #90736

Conversation

olaseni
Copy link
Contributor

@olaseni olaseni commented May 15, 2024

GB is making some new adjustments. See this tracking issue for more context.

At this point, in this commit and PR, the Summary section has been slightly re-imagined and is no longer expansible.

To this end, the concerned e2e tests will be slightly adjusted as the atomic nightlies are beginning to fail. We'll check the existence of that section before we attempt to expand it. This fixes the issue temporarily. Eventually the line to expand that section should be removed in the future.

======= Failed test run #1 ==========
  locator.getAttribute: Timeout 30000ms exceeded.
  =========================== logs ===========================
  waiting for locator('body.block-editor-page').locator('[aria-label="Editor settings"] .components-panel__body-title button:has-text("Summary")')
  ============================================================
  at EditorSettingsSidebarComponent.getAttribute (/home/teamcity-1/buildAgent/work/c4a9d5b38c1dacde/packages/calypso-e2e/src/lib/components/editor-settings-sidebar-component.ts:149:31)
      at EditorSettingsSidebarComponent.expandSection (/home/teamcity-1/buildAgent/work/c4a9d5b38c1dacde/packages/calypso-e2e/src/lib/components/editor-settings-sidebar-component.ts:122:8)
      at EditorPage.schedule (/home/teamcity-1/buildAgent/work/c4a9d5b38c1dacde/packages/calypso-e2e/src/lib/pages/editor-page.ts:756:3)
      at Object.<anonymous> (/home/teamcity-1/buildAgent/work/c4a9d5b38c1dacde/test/e2e/specs/editor/editor__schedule.ts:66:4)

Testing instructions

Running e2e tests for Calypso

cd test/e2e
yarn jest --clearCache && yarn build  # clear build cache before each run


GUTENBERG_NIGHTLY=1 TEST_ON_ATOMIC=true yarn jest specs/editor/editor__schedule.ts
VIEWPORT_NAME=mobile GUTENBERG_NIGHTLY=1 TEST_ON_ATOMIC=true yarn jest specs/editor/editor__schedule.ts

GUTENBERG_NIGHTLY=1 TEST_ON_ATOMIC=true yarn jest specs/editor/editor__page-basic-flow.ts
VIEWPORT_NAME=mobile GUTENBERG_NIGHTLY=1 TEST_ON_ATOMIC=true yarn jest specs/editor/editor__page-basic-flow.ts

@olaseni olaseni self-assigned this May 15, 2024
@matticbot
Copy link
Contributor

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.

@olaseni olaseni requested a review from fredrikekelund May 15, 2024 09:46
@matticbot 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 15, 2024
Copy link
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

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

This works as expected 👍 I left one inline remark where I'll leave the decision to you, @olaseni.

Moreover, for the testing instructions, it looks like the environment variable should be VIEWPORT_NAME and not VIEWPORT.

*
* @param {string} name Name of section to be expanded.
*/
async expandSectionIfExists( name: string ): Promise< void > {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not saying this approach is wrong, but I'm still curious if you considered adding something like an expandSummary method for this specific case? I'm guessing we don't foresee any specific opportunities to reuse expandSectionIfExists, and having a dedicated expandSummary method might clarify that it can be removed in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I'm still curious if you considered adding something like an expandSummary method for this specific case?

Sounds more to-the-point. I like it.

@olaseni
Copy link
Contributor Author

olaseni commented May 15, 2024

Moreover, for the testing instructions, it looks like the environment variable should be VIEWPORT_NAME and not VIEWPORT.

Instructions updated.

I keep pasting those instructions around with little adjustments because there are mostly similar. I must have missed it in one of those instances.

@olaseni olaseni mentioned this pull request May 15, 2024
13 tasks
@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/e2e-update-tests-to-compensate-for-post-summary-that-is-no-longer-collapsible on your sandbox.

@olaseni olaseni merged commit e699a69 into trunk May 15, 2024
11 checks passed
@olaseni olaseni deleted the update/e2e-update-tests-to-compensate-for-post-summary-that-is-no-longer-collapsible branch May 15, 2024 12:17
@github-actions 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 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants