Closed Bug 1394777 Opened 7 years ago Closed 7 years ago

Perma-red browser_UsageTelemetry_content_aboutHome.js

Categories

(Firefox :: New Tab Page, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: andreio, Assigned: andreio)

References

Details

User Story

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb966566398df33bde096c2a7a75287c3e5c8aa&selectedJob=125153766

Attachments

(1 file)

Test browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js fails with browser.newtabpage.activity-stream.aboutHome.enabled;true because it waits for `AboutHomeLoadSnippetsCompleted` events. We want to change this in order to support Activity Stream on about:home.
Assignee: nobody → andrei.br92
User Story: (updated)
Comment on attachment 8902226 [details]
Bug 1394777 - Add test for Activity Stream about:home search.

https://reviewboard.mozilla.org/r/173748/#review179236

It looks like Dexter wrote (bug 1303333) and fixed some intermittents (bug 1313825) related to this test. I'm not sure if removing the listener is the right approach especially if it causes the test to intermittently fail, but that you should be able to try.

Dexter, how should we get this test to pass with Activity Stream enabled on about:home? It does have a search box reusing the same contentSearchUI.js, and this test doesn't seem like it's actually using snippets, so the waiting for event might not be relevant?
Flags: needinfo?(alessio.placitelli)
(In reply to Ed Lee :Mardak from comment #2)
> Comment on attachment 8902226 [details]
> Bug 1394777 - Wait for search input to render before starting about:home
> telemetry test.
> 
> https://reviewboard.mozilla.org/r/173748/#review179236
> 
> Dexter, how should we get this test to pass with Activity Stream enabled on
> about:home? It does have a search box reusing the same contentSearchUI.js,
> and this test doesn't seem like it's actually using snippets, so the waiting
> for event might not be relevant?

The rationale behind waiting for "AboutHomeLoadSnippetsCompleted" was to be completely sure that the about:home page was completely loaded and ready before doing anything. Any better way to do this would make that listener useless and ok to drop. However, I'm not sure if

> await ContentTask.spawn(tab.linkedBrowser, null, async function() {
>   await ContentTaskUtils.waitForCondition(() => content.document.querySelector("input"));
> });

is the best way to do so, as I don't actively work on front-end code. :mak might give a better opinion on that part (thanks :mak :D)!

Regardless of the chosen approach, I'd try to run the failing test 10-20 times on try and see that it stays green before landing this. Also, this seems to be related to bug 1347693.
Flags: needinfo?(alessio.placitelli) → needinfo?(mak77)
AboutHomeLoadSnippetsCompleted is basically only needed if the code cares to work with snippets, and as Dexter said, it could be used as a way to tell "even the slowest thing on the page has finished loading".

In this case, I think we only care about search.
About:home setups search like this:
1. on "pageshow" event it invokes setupSearch
2. setupSearch creates ContentSearchUIController

It also sends a custom "AboutHomeLoad" event used by tab-content.js to setup various things, but those look unrelated to search and thus we may not care.

Thus, just for the search functionality, I assume for both pages we could executeSoon after the "pageshow" event?
Flags: needinfo?(mak77)
Depends on: 1381460
I've changed the patch to consider Activity Stream about:home separate from the current about:home. This is not yet ready for review, it's waiting either on https://github.com/mozilla/activity-stream/pull/3325 or 1381460 because currently the source and healthreportkey of Activity Stream about:home are not correct.
Depends on: 1396272
No longer depends on: 1381460
Blocks: 1394533
Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada691e0088240b52f76a1d83a364067ac1931db looks good.
Mardak, can you please have another look at the patch when you get the time?
Flags: needinfo?(edilee)
I retriggered the bc runs that included browser_UsageTelemetry_content_aboutHome. Although the try run only had linux64 runs.

I triggered a try run from mozreview: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac7846a8bf3
Comment on attachment 8902226 [details]
Bug 1394777 - Add test for Activity Stream about:home search.

https://reviewboard.mozilla.org/r/173750/#review180854

Looks good locally and on try
Attachment #8902226 - Flags: review?(edilee) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a1c4ec3f36f6
Add test for Activity Stream about:home search. r=Mardak
Flags: needinfo?(edilee)
https://hg.mozilla.org/mozilla-central/rev/a1c4ec3f36f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1396654
Depends on: 1347693
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.