Flaky E2E: Tracks "wpcom_block_editor_post_publish_add_new_click" event

worldomonation commented 2 years ago

Spec file

wp-editor__tracks-events.ts

TeamCity ID

7079831

Logs

frame.evaluate: Execution Context is not available in detached frame "https://e2eflowtestinggutenbergsimple.wordpress.com/wp-admin/post.php?post=7180856&action=edit" (are you trying to evaluate?)
at getEventsStack (/home/teamcity-2/buildAgent/work/c4a9d5b38c1dacde/test/e2e/lib/gutenberg/tracking/playwright-utils.ts:24:21)
    at getLatestEvent (/home/teamcity-2/buildAgent/work/c4a9d5b38c1dacde/test/e2e/lib/gutenberg/tracking/playwright-utils.ts:19:22)
    at Object.<anonymous> (/home/teamcity-2/buildAgent/work/c4a9d5b38c1dacde/test/e2e/specs/specs-playwright/wp-editor__tracks-events.ts:45:17)

worldomonation commented 2 years ago

@p-jackson Hi 👋 I know you originally implemented this back in https://github.com/Automattic/wp-calypso/pull/55697 and it appears that this test is one of the two that are permanently failing dating back to November 20.

I'm trying a few things to get this working again, but I am only able to get an undefined value for the event no matter what I try:

    expect(received).toMatchObject(expected)

    Matcher error: received value must be a non-null object

    Received has value: undefined

      54 |      // await gutenbergEditorPage.clickPostPublishAddNewButton( { noWaitAfter: true } );
      55 |
    > 56 |      expect( event ).toMatchObject( [
         |                      ^
      57 |          'wpcom_block_editor_post_publish_add_new_click',
      58 |          expect.objectContaining( {
      59 |              _en: 'wpcom_block_editor_post_publish_add_new_click',

      at Object.<anonymous> (specs/specs-playwright/wp-editor__tracks-events.ts:56:19)

  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'length')

      23 | export async function getLatestEvent( frame: Frame ): Promise< TracksEvent | undefined > {
      24 |  const stack = await getEventsStack( frame );
    > 25 |  return stack.length ? stack[ 0 ] : undefined;
         |               ^
      26 | }
      27 |
      28 | /**

      at getLatestEvent (../../packages/calypso-e2e/src/lib/utils/tracks.ts:25:15)
          at runMicrotasks (<anonymous>)
      at Page.<anonymous> (specs/specs-playwright/wp-editor__tracks-events.ts:50:12)

  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'length')

      23 | export async function getLatestEvent( frame: Frame ): Promise< TracksEvent | undefined > {
      24 |  const stack = await getEventsStack( frame );
    > 25 |  return stack.length ? stack[ 0 ] : undefined;
         |               ^
      26 | }
      27 |
      28 | /**

      at getLatestEvent (../../packages/calypso-e2e/src/lib/utils/tracks.ts:25:15)
      at Page.<anonymous> (specs/specs-playwright/wp-editor__tracks-events.ts:50:12)

  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'length')

      23 | export async function getLatestEvent( frame: Frame ): Promise< TracksEvent | undefined > {
      24 |  const stack = await getEventsStack( frame );
    > 25 |  return stack.length ? stack[ 0 ] : undefined;
         |               ^
      26 | }
      27 |
      28 | /**

      at getLatestEvent (../../packages/calypso-e2e/src/lib/utils/tracks.ts:25:15)
      at Page.<anonymous> (specs/specs-playwright/wp-editor__tracks-events.ts:50:12)

I see that when the Add New Post button is clicked that it first sends a request to https://e2eflowtestinggutenbergsimple.wordpress.com/wp-admin/admin-ajax.php?frame-nonce=<hash>. This causes the frame to be detached/navigated away.

Following the first request is a new request to https://e2eflowtestinggutenbergsimple.wordpress.com/wp-admin/post-new.php?post_type=post, but at this point it looks like the frame is already detached.

My question is: is this tracks event still present and expected to fire?

worldomonation commented 2 years ago

Other things I've tried:

wrapping the call in a Promise.all()
using page.route() to abort all requests arising from the click

p-jackson commented 2 years ago

My question is: is this tracks event still present and expected to fire?

Yes just tested it now manually and see that the wpcom_block_editor_post_publish_add_new_click event is still firing.

I remember this being a pretty hacky implementation to begin with! Not surprised that it's broken now. However we really would like to know if this tracks event breaks due to a Gutenberg upgrade, so the e2e test is appropriate.

What if we came up with a more place to stash the event stack? At the moment it's just a global variable that gets wiped on page transition. What if we saved it in window.sessionStorage? That way it'll only be cleared when we explicitly ask for it to be cleared in the test, and it should survive page transitions.

worldomonation commented 2 years ago

I remember this being a pretty hacky implementation to begin with! Not surprised that it's broken now. However we really would like to know if this tracks event breaks due to a Gutenberg upgrade, so the e2e test is appropriate.

What if we came up with a more place to stash the event stack? At the moment it's just a global variable that gets wiped on page transition. What if we saved it in window.sessionStorage? That way it'll only be cleared when we explicitly ask for it to be cleared in the test, and it should survive page transitions.

While I'm sure given enough time I would be able to come up with a fix, I think making this event persist across pages should definitely simplify this spec and make it much more reliable!

Playwright is able to execute any arbitrary JavaScript via page.evaluate or frame.evaluate (as you probably know) so having the value stored in a slightly more persistent manner would help eliminate timing-related flakes.

I realize you're likely quite up to your neck in blogger experience matters at the moment. Should I file a task, or would you be okay using this issue? I'd like to ensure that you are able to quickly get this done once you have the bandwidth.

In the meantime, I would look at either muting or temporarily stopping the test, if that's acceptable. This is one of the two specs causing the build to permafail and I'd like to see it return to a passing state.

p-jackson commented 2 years ago

This GH issue is fine. We've got a column on our board where I can put "we should really get round to fixing this" things, so I've put it there. Happy for it to be muted till then.

worldomonation commented 2 years ago

Thanks @p-jackson. I have muted the tests in TeamCity for now, and if required will stop it from executing altogether.

Automattic / wp-calypso