1797815 - Update Firefox View callout tests to use the withFirefoxView helper or similar

Status: NEW

(Firefox :: Messaging System, task, P2)

Description

[Hanna Jones [:hjones]]

For Bug 1780441 we are moving away from having our tests open about:firefoxview in a regular tab and instead standardizing on using the withFirefoxView helper. This more closely reflects how users actually open/interact with Firefox View.

It looks like a lot of the callout tests use BrowserTestUtils.withNewTab to open Firefox View in a tab in the current browser. These should also be updated to use the withFirefoxView helper, or something that uses similar mechanics to open the tab.

If using withFirefoxView it seems likely that browser.firefox-view.view-count will have to get set/reset for each test as it gets incremented when the fx view tabstrip button is clicked. (That might be obvious, but when I naively attempted to update some callout tests I was surprised that the fourth test to run always failed).

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Could that could be done automatically inside withFirefoxView itself?

Comment 2

[Hanna Jones [:hjones]]

(In reply to Dan Mosedale (:dmosedale, :dmose) from comment #1)

Could that could be done automatically inside withFirefoxView itself?

Yeah I don't see why not, it could be done by default and/or handled similarly to resetFlowManager.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

In Slack a while back, :aminomancer pointed out some down sides to moving forward with this bug. With his permission, I'm pasting his comments here:

changing all those fx view tests to use FirefoxViewHandler.openTab complicates things since that method does more than just open a new tab. it also doesn't attach a listener and it makes sure you can't open more than 1 of them at the same time

where what we're usually doing right now is BrowserTestUtils.withNewTab which takes a content callback and stuff

and the only reason to switch those methods anyway is because the opening behavior might be prone to change. but at this point the URL has already been changed once and is unlikely to change again, and anything that is likely to change in the behavior will still break our tests anyway

:aminomancer, it'd be helpful to hear your thoughts on what is likely to make sense at this point

Removing the iteration, so we can discuss it at the next triage meeting if it's not already prioritized by then.

Comment 4

[Shane Hughes [:aminomancer]]

It looks like withFirefoxView avoids some of the downsides, e.g. it takes a function to execute on the fx view browser. And I do see the upside of standardizing it. Currently, some of our tests rely on opening multiple fx view tabs (something the FirefoxViewHandler prohibits), because they are testing the ability to sync the feature tour state between instances (but the large majority of our tests don't do this).

One of my main concerns at the time we initially discussed this was that using the FirefoxViewHandler increments the view-count pref, which affects targeting for feature callout reminders. And since these tests are already so finicky with frequent intermittent failures, I wanted to keep them as simple as possible. Since then, Meg did some awesome work improving the reliability of those tests. The intermittent failures were, we think, mostly due to targeting weirdness. Impressions or blocked messages not getting cleaned up for example. Meg basically refactored the tests to use sinon.JS to force the desired messages to be shown. So, impressions, blocking, and the view-count pref should no longer affect whether a message is shown. Which means we have a lot more leeway in general.

With those changes, I wouldn't argue with updating all the tasks that open only a single fx view tab to use withFirefoxView instead of BrowserTestUtils.withNewTab (and leaving alone the ones that test syncing, for example). Since many of these tests are already frequently failing, I think we would treat this as a P3 or P4.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Thanks, :aminomancer, that was super helpful.

For the tests that stay with BrowserTestUtils.withNewTab, I'd suggest adding a comment about why they're not using withFirefoxView, so that future maintainers can have context around how the tests should evolve.