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 Utils: Try improved stability for transformBlockTo #18771

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 26, 2019

This pull request attempts to try to resolve some intermittent E2E test failures, where test failures often occur in the transformBlockTo utility.

Example:

  ● Quote › can be created by converting a paragraph
    TimeoutError: waiting for selector ".block-editor-block-list__block[aria-label="Block: Quote"]" failed: timeout 30000ms exceeded
      at new WaitTask (../../node_modules/puppeteer/lib/DOMWorld.js:549:28)
      at DOMWorld._waitForSelectorOrXPath (../../node_modules/puppeteer/lib/DOMWorld.js:478:22)
      at DOMWorld.waitForSelector (../../node_modules/puppeteer/lib/DOMWorld.js:432:17)
      at Frame.waitForSelector (../../node_modules/puppeteer/lib/FrameManager.js:627:47)
      at Frame.<anonymous> (../../node_modules/puppeteer/lib/helper.js:112:23)
      at Page.waitForSelector (../../node_modules/puppeteer/lib/Page.js:1125:29)
      at page (../e2e-test-utils/build/@wordpress/e2e-test-utils/src/transform-block-to.js:18:8)
      ...

While I was never able to reproduce the issue locally, I attempted to make revisions based on the above information, specifically the fact that it became stuck at the last line of the function (waitForSelector) means that all of the preceding interactions were successful (display the switcher, find a button with the text "Quote" and click it).

The proposed changes aren't guaranteed to improve the tests, but are based on the following observations:

  • The previous XPath selector was selecting the span of the button on which to perform the click, not the button itself where the event handler is bound. With these changes, it now matches the button.
    • Hypothesis: Maybe it was clicking the span but not registering as a click on the button?
  • The previous XPath selector would match any button anywhere on the page that happened to have a span descendent which included the text "Quote". With these changes, the XPath selector is limited to match within the switcher popover.
    • Hypothesis: It was potentially clicking some other button on the page; though, again, in my local testing there were no other buttons which match this selector.
  • The previous XPath selector would use [contains(text(), '...')] to match the text. With these change, the text is now matched using [.='...'].
    • Hypothesis: The behavior of text() is reasonably strict about placement of text nodes. Perhaps there were some unexpected conditions where additional DOM nodes might be added? Looking at the code, it doesn't appear it should be the case. But for the purposes of the utility, it's not really concerned with strict conformance. Contrast with the proposed .=, which is both simpler and more lenient about queried text in that it queries on the string concatenation of all text nodes in the parent (reference).

I also included a change to use keyboard commands to navigate to the block toolbar and expand the switcher container, in place of the "mouse wiggle" interaction. As noted, since the function was able to successfully reach the final line of its execution, I don't expect this should have an impact one way or the other, but generally I would find the page.mouse.move approach to be less reliable overall.

@aduth aduth added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] E2E Test Utils /packages/e2e-test-utils labels Nov 26, 2019
@ellatrix
Copy link
Member

Thanks for having a look. I'll take the liberty to restart the build a few times to see if it makes any difference. :)

@ellatrix
Copy link
Member

Unrelated intermittent e2e test failure:


1000FAIL packages/e2e-tests/specs/editor/various/embedding.test.js (24.067s)
1001  ● Embedding content › should render embeds in the correct state
1002
1003    Node is either not visible or not an HTMLElement
1004
1005      29 |             _context.next = 2;
1006      30 |             return page.click('.block-editor-default-block-appender__content');
1007    > 31 | 
1008         | ^
1009      32 |           case 2:
1010      33 |           case "end":
1011      34 |             return _context.stop();
1012
1013      at ElementHandle._clickablePoint (../../node_modules/puppeteer/lib/JSHandle.js:222:13)
1014          at runMicrotasks (<anonymous>)
1015      at ElementHandle.click (../../node_modules/puppeteer/lib/JSHandle.js:283:20)
1016      at DOMWorld.click (../../node_modules/puppeteer/lib/DOMWorld.js:367:5)
1017        -- ASYNC --
1018      at ElementHandle.<anonymous> (../../node_modules/puppeteer/lib/helper.js:111:15)
1019      at DOMWorld.click (../../node_modules/puppeteer/lib/DOMWorld.js:367:18)
1020          at runMicrotasks (<anonymous>)
1021        -- ASYNC --
1022      at Frame.<anonymous> (../../node_modules/puppeteer/lib/helper.js:111:15)
1023      at Page.click (../../node_modules/puppeteer/lib/Page.js:1067:29)
1024      at _callee$ (../e2e-test-utils/build/click-block-appender.js:31:25)
1025      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:45:40)
1026      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:271:22)
1027      at Generator.prototype.<computed> [as next] (../../node_modules/regenerator-runtime/runtime.js:97:21)
1028      at asyncGeneratorStep (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
1029      at _next (../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)
1030      at ../../node_modules/@babel/runtime/helpers/asyncToGenerator.js:32:7
1031
1032

@ellatrix
Copy link
Member

ellatrix commented Nov 27, 2019

  1. Original build: ✅
  2. 2nd build: ✅
  3. 3rd build: ✅
  4. 4th build: ✅
  5. 5th build: ✅

@ellatrix
Copy link
Member

Another unrelated intermittent test failure:


1044FAIL packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js (13.774s)
1045  ● Navigating the block hierarchy › should navigate using the block hierarchy dropdown menu
1046
1047    expect(received).toMatchSnapshot()
1048
1049    Snapshot name: `Navigating the block hierarchy should navigate using the block hierarchy dropdown menu 1`
1050
1051    - Snapshot
1052    + Received
1053
1054    @@ -8,10 +8,12 @@
1055      <!-- wp:column -->
1056      <div class="wp-block-column"></div>
1057      <!-- /wp:column -->
1058      
1059      <!-- wp:column -->
1060    - <div class="wp-block-column"><!-- wp:paragraph -->
1061    - <p>Third column</p>
1062    - <!-- /wp:paragraph --></div>
1063    + <div class="wp-block-column"></div>
1064      <!-- /wp:column --></div>
1065    - <!-- /wp:columns -->"
1066    + <!-- /wp:columns -->
1067    + 
1068    + <!-- wp:image -->
1069    + <figure class="wp-block-image"><img alt=""/></figure>
1070    + <!-- /wp:image -->"
1071
1072      59 | 		await page.keyboard.type( 'Third column' );
1073      60 | 
1074    > 61 | 		expect( await getEditedPostContent() ).toMatchSnapshot();
1075         | 		                                       ^
1076      62 | 	} );
1077      63 | 
1078      64 | 	it( 'should navigate block hierarchy using only the keyboard', async () => {
1079
1080      at Object.toMatchSnapshot (specs/editor/various/block-hierarchy-navigation.test.js:61:42)
1081          at runMicrotasks (<anonymous>)
1082
1083 › 1 snapshot failed.

@ellatrix ellatrix merged commit 0330af2 into master Nov 27, 2019
@ellatrix ellatrix deleted the try/e2e-transform-stabilization branch November 27, 2019 16:17
@aduth
Copy link
Member Author

aduth commented Dec 5, 2019

Since this does seem to have stabilized things, it gets me wondering what of the specific changes would have had the most impact here, so we can make sure to try to keep it in mind for future tests-authoring.

  • Was it that we shouldn't target inner elements of a clickable button?
  • Was it that we should prefer using keyboard to navigate the page, vs. clicking on elements?
  • Was it the "mouse wiggle" interaction as being unreliable?

I don't have the answer right now, but I think it's worth keeping in mind to try to avoid future issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] E2E Test Utils /packages/e2e-test-utils [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants