Temporarily disable failing tests due to process flip: browser_aboutHome_search_composing.js and browser_aboutHome_search_telemetry.js

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: imjching, Assigned: arai)

Tracking

unspecified
Firefox 63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

10 months ago
Both the tests `browser_aboutHome_search_composing.js` and `browser_aboutHome_search_telemetry.js` rely on the `waitForDocLoadAndStopIt` function.

The current behavior is that the tests add a new progress listener in the `about:home` page and stops the content load whenever there is a network request. The tests then simulate a network load through a mouse click, which attempts to navigate the page and stops the content load. The original `about:home` page will still be displayed to the user because a new load did not occur, allowing subsequent assertions to pass (e.g. checking to ensure that the search bar still has the original value as the one before the navigation).

When we enable the pref to load Activity Stream in a new content process, there will be a process flip whenever we attempt to navigate away from `about:home`. When that happens, calling `waitForDocLoadAndStopIt` will result in an `about:blank` page because the previous session is not persisted during a process flip. After talking to :nika, it seems that this is a known bug and should be fixed by Fission. Theoretically we could just make the tests pass by trying to add some workarounds to prevent navigation, but we don't think it is worth testing a situation that users cannot get into.

I am hoping to ship Bug 1472212 before Fission, but we'll need to acknowledge these two tests and disable them until a fix has been implemented.

Thoughts?

Comment 1

10 months ago
(In reply to Jay Lim [:imjching] from comment #0)
> When we enable the pref to load Activity Stream in a new content process,
> there will be a process flip whenever we attempt to navigate away from
> `about:home`. When that happens, calling `waitForDocLoadAndStopIt` will
> result in an `about:blank` page because the previous session is not
> persisted during a process flip. After talking to :nika, it seems that this
> is a known bug and should be fixed by Fission. Theoretically we could just
> make the tests pass by trying to add some workarounds to prevent navigation,
> but we don't think it is worth testing a situation that users cannot get
> into.
> 
> I am hoping to ship Bug 1472212 before Fission, but we'll need to
> acknowledge these two tests and disable them until a fix has been
> implemented.

Can't we just do:

browser.goBack();
await BrowserTestUtils.browserLoaded(browser, "about:home");

? before attempting to continue to interact with about:home?
Flags: needinfo?(jlim)
Reporter

Comment 2

10 months ago
(In reply to :Gijs (he/him) from comment #1)
> Can't we just do:
> 
> browser.goBack();
> await BrowserTestUtils.browserLoaded(browser, "about:home");
> 
> ? before attempting to continue to interact with about:home?

It seems like that will not work because `browser.goBack()` does not preserve the input value that a user typed into the search bar before the user navigates. One of the tests checks to ensure that the input value is the same: https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/base/content/test/about/browser_aboutHome_search_composing.js#81.
Flags: needinfo?(jlim)

Comment 3

10 months ago
(In reply to Jay Lim [:imjching] from comment #2)
> (In reply to :Gijs (he/him) from comment #1)
> > Can't we just do:
> > 
> > browser.goBack();
> > await BrowserTestUtils.browserLoaded(browser, "about:home");
> > 
> > ? before attempting to continue to interact with about:home?
> 
> It seems like that will not work because `browser.goBack()` does not
> preserve the input value that a user typed into the search bar before the
> user navigates. One of the tests checks to ensure that the input value is
> the same:
> https://searchfox.org/mozilla-central/rev/
> d47c829065767b6f36d29303d650bffb7c4f94a3/browser/base/content/test/about/
> browser_aboutHome_search_composing.js#81.

Aha, OK. In that case, I guess we should intercept the load earlier. We could perhaps stub out the method we end up using to load the content from within the test? That's a bit fragile, but also more unit-test-y.


The other option here is to split the test into two sub-tests that each open about:home and do the composition-y bits. I don't know why the test has to verify that the typed content is still there after first allowing a navigation - practically speaking, that doesn't make a lot of sense...

:arai (seems you worked on this in bug 1115616 though the test got refactored a bunch since then!), can you clarify why the test is checking the input value after blocking navigation, and given that we can't actually block navigation accurately now that we're process switching, can we just omit those checks or split them off into a separate test task that loads about:home itself, or whether that'd be wrong for some reason?
Flags: needinfo?(arai.unmht)
Assignee

Comment 4

10 months ago
(In reply to :Gijs (he/him) from comment #3)
> :arai (seems you worked on this in bug 1115616 though the test got
> refactored a bunch since then!), can you clarify why the test is checking
> the input value after blocking navigation , and given that we can't actually
> block navigation accurately now that we're process switching, can we just
> omit those checks or split them off into a separate test task that loads
> about:home itself, or whether that'd be wrong for some reason?

previously, the selected suggestion string is filled to the input field, and the original test was checking that behavior.
https://hg.mozilla.org/mozilla-central/rev/b683c06bf809#l2.57

but the behavior seems to be changed at some point and now the selected suggestion is *not* filled into the input field,
and now the test is checking the entered value, that doesn't make sense at all.

the test needs to be rewritten to check the resulting page's URL or something, in order to verify that it navigates to
the search result for selected suggestion.

anyway, I'm fine to disable the test meanwhile, given that it's not checking anything useful.
Flags: needinfo?(arai.unmht)
Assignee

Comment 5

10 months ago
try is running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a173485934741249e9d94e14f6a88117cf539b9
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee

Comment 6

10 months ago
* Removed the check for input field
  * Modified searchSuggestionEngine.xml in order to include the search term in query

now the test checks if the request URL contains the search term.
Attachment #8995864 - Flags: review?(gijskruitbosch+bugs)

Comment 7

10 months ago
Comment on attachment 8995864 [details] [diff] [review]
Check the search term in the request URL in tests for about:home.

Review of attachment 8995864 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: browser/base/content/test/about/browser_aboutHome_search_composing.js
@@ +80,4 @@
>        let row = content.document.getElementById("TEMPID");
>        if (row) {
>          row.removeAttribute("id");
>        }

I think you can then remove the entire content task. The tab is removed when we close the tab, so I don't see the point of keeping the temporary ID...
Attachment #8995864 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

10 months ago
(In reply to :Gijs (he/him) from comment #7)
> Comment on attachment 8995864 [details] [diff] [review]
> Check the search term in the request URL in tests for about:home.
> 
> Review of attachment 8995864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: browser/base/content/test/about/browser_aboutHome_search_composing.js
> @@ +80,4 @@
> >        let row = content.document.getElementById("TEMPID");
> >        if (row) {
> >          row.removeAttribute("id");
> >        }
> 
> I think you can then remove the entire content task. The tab is removed when
> we close the tab, so I don't see the point of keeping the temporary ID...

Err, *removing* the temporary ID, that is...

Updated

10 months ago
Priority: -- → P1
Assignee

Comment 9

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbdfc68cc846ddeeb86710a36a40e887e75fa5da
Bug 1478792 - Check the search term in the request URL in tests for about:home. r=Gijs

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbdfc68cc846
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.