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

Remove insert block delay from e2e tests #22377

Merged
merged 2 commits into from
May 15, 2020
Merged

Remove insert block delay from e2e tests #22377

merged 2 commits into from
May 15, 2020

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented May 15, 2020 •
edited
Loading

This delay was added in #21368 to stabilize e2e tests but ideally, it's not needed and may hide real bugs.

@github-actions
Copy link

github-actions bot commented May 15, 2020 •
edited
Loading

Size Change: +12 B (0%)

Total Size: 833 kB

Filename Size Change
build/block-editor/index.js 104 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.59 kB 0 B
build/block-directory/style-rtl.css 764 B 0 B
build/block-directory/style.css 764 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.19 kB 0 B
build/block-library/editor.css 7.19 kB 0 B
build/block-library/index.js 118 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 182 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17 kB 0 B
build/compose/index.js 6.67 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 5.77 kB 0 B
build/edit-navigation/style-rtl.css 709 B 0 B
build/edit-navigation/style.css 708 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad
Copy link
Contributor Author

One interesting remark here is that all the failing tests are for "container blocks" being inserted which suggests clearly that the "async" behavior that was introduced is in the "InnerBlocks" component.

@youknowriad youknowriad marked this pull request as ready for review May 15, 2020 10:05
@youknowriad youknowriad self-assigned this May 15, 2020
@youknowriad youknowriad added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label May 15, 2020
epiqueras
epiqueras approved these changes May 15, 2020
View reviewed changes
Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

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

Makes sense.

@epiqueras epiqueras merged commit bab1bc8 into master May 15, 2020
@epiqueras epiqueras deleted the try/remove-insert-block-delay branch May 15, 2020 17:21
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 15, 2020
@aduth
Copy link
Member

aduth commented May 18, 2020

Based on the code and where in the flow the error occurs, I can't see why this change would be the cause, but noting that I've seen quite a few build failures lately occurring within the insertBlock end-to-end utility updated here.

Example: https://travis-ci.com/github/WordPress/gutenberg/jobs/336142279

FAIL packages/e2e-tests/specs/editor/blocks/quote.test.js (71.035s)

  ● Quote › can be merged into from a paragraph
    TypeError: Cannot read property 'click' of undefined
      at _callee$ (../e2e-test-utils/build/@wordpress/e2e-test-utils/src/insert-block.js:12:8)
      at tryCatch (../../node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (../../node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:274:22)
      at Generator.prototype.<computed> [as next] (../../node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)
          at runMicrotasks (<anonymous>)

aduth
aduth reviewed May 18, 2020
View reviewed changes
packages/e2e-test-utils/src/insert-block.js
@@ -2,7 +2,6 @@
* Internal dependencies
*/
import { searchForBlock } from './search-for-block';
import { getAllBlocks } from './get-all-blocks';

/**
* Opens the inserter, searches for the given term, then selects the first
Copy link
Member

Choose a reason for hiding this comment

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

Noting that the changes in 4d23115 included a revision to the function documentation: "It then waits briefly for the block list to update.". It wasn't reverted here.

@aduth aduth mentioned this pull request May 18, 2020
Closed
@aduth
Copy link
Member

aduth commented May 18, 2020

Based on the code and where in the flow the error occurs, I can't see why this change would be the cause, but noting that I've seen quite a few build failures lately occurring within the insertBlock end-to-end utility updated here.

Moved to issue (with a couple debugging leads) at #22430

@noahtallen
Copy link
Member

Really nice! Glad we were able to fix it. @youknowriad Could you explain why useLayoutEffect fixes the issue?

@noahtallen noahtallen mentioned this pull request May 18, 2020
Merged
10 tasks
@epiqueras
Copy link
Contributor

It's called synchronously unlike useEffect.

@noahtallen
Copy link
Member

noahtallen commented May 18, 2020 •
edited
Loading

Ah, so the e2e tests were querying for bit of DOM that only existed after the side effect executed. And now the effect is executed before that point. Makes sense. 👍 Very helpful to know about that distinction for the future, too.

@epiqueras
Copy link
Contributor

Exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@epiqueras epiqueras epiqueras approved these changes

@noahtallen noahtallen Awaiting requested review from noahtallen

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@ellatrix ellatrix Awaiting requested review from ellatrix

@gziolo gziolo Awaiting requested review from gziolo

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

Assignees

@youknowriad youknowriad

Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
Gutenberg 8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@youknowriad @aduth @noahtallen @epiqueras