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

Update experimental features tests #48164

Merged
merged 10 commits into from
Dec 24, 2020
Merged

Conversation

deBhal
Copy link
Contributor

@deBhal deBhal commented Dec 9, 2020

NOTE: This PR is currently targeted at the add/experimental-block-patterns-tests branch from #48032. Once that PR is merged, we'll rebase against that new trunk instead.

This PR is a followup to #48032 that fixes the experimental features tests and combines the experimental data tests introduced in #48032 together with them in the one file.

There were a few things that were preventing the experimental feature tests from running successfully:

  • The tests need the @parallel tag to get picked up on CircleCI (although they run fine locally without them)
  • The javascript passed to the selenium driver.executeScript() needs a return statement. The driver was returning null no matter what happened in the test environment, leading to false positives.
  • A typo that caused another assert to always pass.

After fixing those, we still see a failure with '__experimentalMainDashboardButton', and I think this might be a legitimate failure, as I can't see interface or __experimentalMainDashboardButton on the global wp property in the editor.

Testing instructions

You can re-trigger the tests to run on CircleCI by removing and re-adding the [Status] Needs e2e Testing label, but if you want to test changes there (like removing the failing check to test the rest) you'll need to push a new branch and add the label there.

You can also run the tests locally from the test/e2e directory in calypso using something like:
env BROWSERSIZE=desktop npx mocha specs/wp-gutenberg-experimental-features-spec.js
(You may or may not need some config decryption magic as described in the Field guide "Automated end-to-end Testing" page)

Also fixes #47839.

@matticbot
Copy link
Contributor

@deBhal deBhal marked this pull request as draft December 9, 2020 10:30
@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.

@deBhal
Copy link
Contributor Author

deBhal commented Dec 11, 2020

Intentional failures show up in https://app.circleci.com/pipelines/github/Automattic/wp-e2e-tests-for-branches/50286/workflows/0d8b070c-3749-4515-ac3b-56ea86e8bea7/jobs/125901 (It actually occurs three times because we've configure 3 tries before giving up):

Notification_Center

@deBhal deBhal force-pushed the fix/experimental-features-test branch from b7995b7 to bf0dde4 Compare December 14, 2020 10:12
@deBhal deBhal changed the base branch from trunk to add/experimental-block-patterns-tests December 14, 2020 10:15
@deBhal
Copy link
Contributor Author

deBhal commented Dec 14, 2020

@p-jackson: Is wp.interface being null a legitimate test failure?

@deBhal deBhal requested a review from p-jackson December 14, 2020 10:45
@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 Dec 14, 2020
@p-jackson
Copy link
Member

Good find! I think these tests have been broken but since it didn't have the @parallel we didn't notice!

There's actually no such thing as wp.interface at runtime; the @wordpress/interface package is bundled at build time, it doesn't expose itself as a global. So the __experimentalMainDashboardButton test isn't actually appropriate for this e2e. (at least until Gutenberg 9.5 when the experimental feature moves to another package, but that's not being addressed in this PR)

The __experimentalMainDashboardButton case is being tested in this unit test: apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/test/attach-sidebar.test.ts

I think the best course of action is just to delete the @wordpress/interface entry from the EXPERIMENTAL_FEATURES object.

@p-jackson
Copy link
Member

Yep, just found the line was added in #47712 but CI would have never run the test.

@deBhal deBhal force-pushed the add/experimental-block-patterns-tests branch from fa82a4f to 515c09f Compare December 15, 2020 00:47
@deBhal deBhal force-pushed the fix/experimental-features-test branch from bf0dde4 to e431baf Compare December 16, 2020 07:38
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D54345-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@deBhal deBhal changed the base branch from add/experimental-block-patterns-tests to trunk December 16, 2020 23:46
@deBhal deBhal force-pushed the fix/experimental-features-test branch from f0f0d18 to 5a93887 Compare December 22, 2020 01:46
@deBhal
Copy link
Contributor Author

deBhal commented Dec 23, 2020

I've been trying to figure out why editSite is not is not there, and while it's still a mystery, It does seem to be a legitimate failure - there's no wp.editSite in the editor contexts I've checked.

I tried to figure out how that global wp site gets populated, but failed to penetrate the magic. @wordpress/edit-post does get added to the global object, but @wordpress/edit-site doesn't, and I can't see the difference or find where it's specified.

The actual target property seems odd too. The version of edit-site that we're pulling (v1.15.6) in doesn't expose the property, but the latest gutenberg master verstion is up to v1.16, and it does expose it, so maybe we're ahead of the curve rather than behind?

Also weird: We use the property in apps/wpcom-block-editor/src/calypso/features/iframe-bridge-server.js, but when I dumped out the editSitePackage property at run-time it was null. I wonder if this is related to the npm package magic that we use - there's no edit-site package in apps/wpcom-block-editornode_modules, it pulls in the version from the calypso root, and I wonder if the the require() call is unaware of this magic.

Anyway, I've spent way too much time trying to figure this out already, so for this PR I'm just dropping the line. I'll create an issue with these findings and see if I can find someone to pick it up.

@deBhal deBhal changed the title Try: Update experimental features tests Update experimental features tests Dec 23, 2020
@deBhal deBhal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed DO NOT MERGE [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 23, 2020
@deBhal deBhal requested review from fullofcaffeine and a team December 23, 2020 23:35
@deBhal deBhal marked this pull request as ready for review December 23, 2020 23:36
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Ran locally as well:

    ✓ Can log in (29467ms)
    Can find experimental package features
      ✓ "blockEditor" package should be available in the global window object
      ✓ __experimentalBlock should be available in @wordpress/block-editor
      ✓ __experimentalInserterMenuExtension should be available in @wordpress/block-editor
      ✓ "date" package should be available in the global window object
      ✓ __experimentalGetSettings should be available in @wordpress/date
      ✓ "components" package should be available in the global window object
      ✓ __experimentalNavigationBackButton should be available in @wordpress/components
    Experimental data we depend on is available
      ✓ is iterable: wp.data.select( 'core/editor' ).getEditorSettings().__experimentalBlockPatterns


  9 passing (32s)

@deBhal deBhal merged commit 5881cb3 into trunk Dec 24, 2020
@deBhal deBhal deleted the fix/experimental-features-test branch December 24, 2020 02:23
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 24, 2020
@deBhal
Copy link
Contributor Author

deBhal commented Dec 24, 2020

I've popped my notes from trying to figure out the edit-site thing into #48585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure wp-gutenberg-experimental-features-spec.js spec runs
4 participants