E2E: Fix tracking specs against GB v14.8.x

[WunderBart]

Warning
Do not merge before Gutenberg v14.8 is in production.

Context:

• p4TIVU-am0-p2
• p4TIVU-amj-p2

Proposed Changes

Fix tracking specs against the GB v14.8.x upgrade:

• specs/editor-tracking/editor-tracking__block-inserted-event.ts
• specs/editor-tracking/editor-tracking__other-block-events.ts
• specs/editor-tracking/editor-tracking__toolbar-events.ts
• specs/editor-tracking/editor-tracking__global-styles-events.ts
• specs/editor-tracking/editor-tracking__fse-template-events.ts
• specs/editor-tracking/editor-tracking__document-actions-events.ts
• specs/editor-tracking/editor-tracking__pattern-events.ts

Testing Instructions

The above suites should be passing against edge sites in the CI.

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-54401

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-54401&env=jetpack

[matticbot]

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.

[WunderBart]

@dpasque @Addison-Stavlo

@noahtallen noticed (p1672270550575539/1672268819.313079-slack-CBTN58FTJ) that the tracking specs are failing for the Gutenberg v14.8 upgrade, but I suspect they've been broken for a while now. Failures from this PR seem to have been piling up since the last few upgrades. For example, the Details feature was (re)moved in v14.5, which would mean those specs should be failing since ~Nov 18th.

What's the policy on running tracking specs? Are they considered blocking/required for the Gutenberg upgrade? I wasn't sure about that, so we've postponed the v14.8 upgrade (p4TIVU-amj-p2) mostly because of those failures.

/cc @worldomonation @fullofcaffeine

[WunderBart]

There's still one failure that I didn't know how to address because it seems to actually point to a missing event:

• https://teamcity.a8c.com/buildConfiguration/calypso_WPComTests_editor_tracking_simple_edge_desktop/9236189?buildTab=tests&status=failed&expandedTest=build%3A%28id%3A9236189%29%2Cid%3A1119.

Could you take a look @Addison-Stavlo / @dpasque? 🙏

[WunderBart]

The specs/editor/editor__schedule.ts: Editor: Schedule: Schedule: future spec is failing, but it's unrelated to this PR. I will address it in a separate one.

[dpasque]

@WunderBart -- thank you so much for working on these! 🙇

What's the policy on running tracking specs? Are they considered blocking/required for the Gutenberg upgrade? I wasn't sure about that, so we've postponed the v14.8 upgrade (p4TIVU-amj-p2) mostly because of those failures.

I can help weigh in here!

Two helpful threads that I think should give a lot of context:
p1654549794215299-slack-C1A1EKDGQ
pdqkMK-mx-p2

It's been hard for a while to dial in the right relevant priority for these Tracks metric! They're valuable, but still require a decent amount of maintenance, and placing that among the many other priorities we have on dotcom is hard! I know @Addison-Stavlo's team has done a lot of work keeping up on the changes, but there will likely continue to be breakages over time.

As discussed in the Slack thread, we opted to move them to their own build and make them non-blocking for Gutenberg upgrades. Since they usually only point to failures that don't affect the user experience, we didn't want to hold up those releases. The idea was that Gutenberg could still be deployed with a failing test, and then we would ping Cylon/KitKat to notify of the failure and we would go back and make updates. However, looking at the volume of what needed to be fixed here, I'm wondering if that will be sustainable. 😬

I've been thinking a lot about Eric's comment here: pdqkMK-mx-p2#comment-382. @Addison-Stavlo -- do you think it would make sense to pare down the E2E tests to only the Tracks metrics that are actively being used? Is that too dramatic? I'm just trying to consider other ways to minimize the maintenance here!

[dpasque]

Code changes look good btw! 👍 One question for you about the page closures.

[dpasque]

Question: Do we need this here? Aren't all the pages closed up by our custom jest environment? AFAIK, we usually don't need to close up pages manually during tests.

[dpasque]

And actually, I think that may mess with capturing screenshots and videos, because we iterate through open pages on teardown and grab the screenshots and videos. But if the page is already closed, the teardown logic won't find it!

[WunderBart]

Perfect catch! I've just realized that the after hook is fired even if the block fails, making it the wrong spot to put the page closing at because it indeed would prevent the artifacts from being created. 💥 In the case of suites like these, though, where there are multiple describe blocks, I'd still close the page at the end of every block (only in a dedicated step instead of the after hook). Otherwise, the recordings will not be stopped even for successful blocks and will be uploaded along with failed ones. For example, this will upload three sets of artifacts, of which only the last one is relevant:

// foo.spec.js

describe({
  // opens page #1, succeeds
})
describe({
  // page #1 is open
  // opens page #2, succeeds
})
describe({
  // page #1 is open
  // page #2 is open
  // opens page #3, fails
})

// Artifacts for page #1, #2 and #3 are uploaded.

This is true for our current env setup, so alternatively, maybe we could make some changes not to create artifacts for successful blocks? Let me know what you think, @dpasque!

---

Accidentally, while testing if what I've described above is true, I've uncovered a bug in our env setup that causes each artifact (trace/recording/image/etc.) to overwrite the previous one. It happens because we're creating the artifact name outside of the context→page iteration blocks, which causes each context & page have the 0 index. I'll make a fix in another PR!

[WunderBart]

PR here: #71623

[dpasque]

This is true for our current env setup, so alternatively, maybe we could make some changes not to create artifacts for successful blocks? Let me know what you think, @dpasque!

Yes for sure, I've long wanted to do this, but never just set aside to knock it out! I think that would be a nice improvement over our current setup 👍

[worldomonation]

comment: A counterpoint is that we may have a situation where the previous top-level describe blocks may have passed but it inadvertently put the webapp in a condition that leads to an immediate failure in the next top-level describe block.

In the hypothetical spec with three describe blocks above, it is possible that an overlay appears between the last step of the second block and the beginning of the third block on the same page.

Granted, it is little bit of a cherry-picked scenario, I will admit that!

[dpasque]

There's still one failure that I didn't know how to address because it seems to actually point to a missing event:
https://teamcity.a8c.com/buildConfiguration/calypso_WPComTests_editor_tracking_simple_edge_desktop/9236189?buildTab=tests&status=failed&expandedTest=build%3A%28id%3A9236189%29%2Cid%3A1119.
Could you take a look @Addison-Stavlo / @dpasque? 🙏

I fixed this in 7d4b937 -- it looks like we are now tracking the page list item block as part of the event now. 👍

Also, it looks like the new sidebar design didn't play well with our pattern for deleting template parts, and we weren't deleting more than the first template part! I fixed that in d90d6ca.

With that, it looks like the CI Edge build is green! 🙌

cc @WunderBart and @Addison-Stavlo (fyi that this no longer needs investigation! 👍 )

[WunderBart]

Thank you for all the context and fixing that last bit, @dpasque – much appreciated! 🙌
TBH, I suspected that these might not be required, but the fact that there were so many failures made me think that we need to take a closer look regardless before we continue upgrading. Having said that, I'll move on with the upgrade now that it's all clear and green. Thanks again! 🙇

[dpasque]

Approving pending the removal of the afterAll hooks! 👍 🚀

[WunderBart]

@dpasque, I need some more help with this one if you have a spare cycle! 🙏 Looks like the Template Modal has been refactored with one of the recent upgrades, and some specs started failing because of it. I'm having a hard time figuring out the right fix for this. Would you be able to take a look?

[WunderBart]

Thanks for taking this one over the finish line @dpasque @fullofcaffeine! 🙇