Closed Bug 1381684 Opened 2 years ago Closed 2 years ago

Intermittent browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_lastAccessed.js | lastAccessed timestamps are recent and in the right order -

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: bsilverberg)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell unknown])

Attachments

(1 file)

Priority: -- → P5
I will look into this one.
Assignee: nobody → bob.silverberg
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P3
Comment on attachment 8923496 [details]
Bug 1381684 - Fix intermittent browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js,

https://reviewboard.mozilla.org/r/194634/#review199724

Either add a comment to the onUpdated call to explain why its used, or change to my pefered option below.

::: browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js:16
(Diff revision 1)
> -          return;
> -        }
> -
> -        let [tab1] = await browser.tabs.query({url: "https://example.com/?1"});
> -        let [tab2] = await browser.tabs.query({url: "https://example.com/?2"});
> -
> +      const START_TIME = Date.now();
> +
> +      let tab1 = await browser.tabs.create({url: URL1});
> +      let tab2 = await browser.tabs.create({url: URL2});
> +
> +      browser.tabs.onUpdated.addListener(async (id, changeInfo, tab) => {

Overall I don't like the onUpdated use because I expect it to be doing something it's not which leads to confusion and having to think this through more.  Somebody down the road is going to look at it with the same confusion.  Basically, it's just a long delay for getting "now".

I think just adding waitForLoad or waitForStateStop options to both openNewForegroundTab would fix the timing issue better.

I do like splitting out the assertions.
Attachment #8923496 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8923496 [details]
Bug 1381684 - Fix intermittent browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js,

https://reviewboard.mozilla.org/r/194634/#review199724

> Overall I don't like the onUpdated use because I expect it to be doing something it's not which leads to confusion and having to think this through more.  Somebody down the road is going to look at it with the same confusion.  Basically, it's just a long delay for getting "now".
> 
> I think just adding waitForLoad or waitForStateStop options to both openNewForegroundTab would fix the timing issue better.
> 
> I do like splitting out the assertions.

I tried that on Windows, setting both `waitForLoad` and `waitForStateStop` to true (waitForLoad defaults to true anyway) and it did not fix the problem. I also tried using `BrowserTestUtils.loadURI` together with `BrowserTestUtils.browserLoaded`, as I'd done before in browser_ext_sessions_getRecentlyClosed.js [1], but still, on Windows, I'm seeing that lastAccessed of tab2 is later than the time I record for `now`. I agree that all I'm doing with the onUpdated is introducing a wait, but I'm not sure what else I can wait for. Do you have any other suggestions?

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed.js#13-14
Did you try {skipAnimation: true} in the openNewForegroundTab call?

Aside from that, I wonder about what we're actually testing here and why.  There are already tests for lastAccessed values (which btw seem to have worked around the same problem, see browser_lastAccessedTab.js).  Given that, this just feels repetitive.  Anyway, also given that other similar tests have similar timing issues with lastAccessed and Date.now, I think we should just land this and see how it goes.
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
Sorry Shane, I totally missed your reply to this last week.

> Did you try {skipAnimation: true} in the openNewForegroundTab call?
>

openNewForegroundTab doesn't support {skipAnimation: true}, but addTab does, so I tried that, along with BrowserTestUtils.browserLoaded and it seems to have addressed the intermittent, at least locally for me.

I'll make that change and submit it to try and see how it looks.
Comment on attachment 8923496 [details]
Bug 1381684 - Fix intermittent browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js,

https://reviewboard.mozilla.org/r/194634/#review199724

> I tried that on Windows, setting both `waitForLoad` and `waitForStateStop` to true (waitForLoad defaults to true anyway) and it did not fix the problem. I also tried using `BrowserTestUtils.loadURI` together with `BrowserTestUtils.browserLoaded`, as I'd done before in browser_ext_sessions_getRecentlyClosed.js [1], but still, on Windows, I'm seeing that lastAccessed of tab2 is later than the time I record for `now`. I agree that all I'm doing with the onUpdated is introducing a wait, but I'm not sure what else I can wait for. Do you have any other suggestions?
> 
> [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed.js#13-14

As discussed in the bug, I found a different solution that seems to fix the intermittent.
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c908b2bffe6e
Fix intermittent browser/components/extensions/test/browser/browser_ext_tabs_lastAccessed.js, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/c908b2bffe6e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: qe-verify-
Depends on: 1416039
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.