Skip to content

Remove insert block delay from e2e tests#22377

Merged
epiqueras merged 2 commits into
masterfrom
try/remove-insert-block-delay
May 15, 2020
Merged

Remove insert block delay from e2e tests#22377
epiqueras merged 2 commits into
masterfrom
try/remove-insert-block-delay

Conversation

@youknowriad
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

youknowriad commented May 15, 2020

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
Copy Markdown
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
Copy Markdown
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
Comment thread packages/e2e-test-utils/src/insert-block.js
import { getAllBlocks } from './get-all-blocks';

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

@aduth aduth May 18, 2020

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
Copy Markdown
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
Copy Markdown
Member

noahtallen commented May 18, 2020

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
Copy Markdown
Contributor

epiqueras commented May 18, 2020

It's called synchronously unlike useEffect.

@noahtallen
Copy link
Copy Markdown
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
Copy Markdown
Contributor

epiqueras commented May 18, 2020

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