Closed
Bug 1344087
Opened 7 years ago
Closed 7 years ago
The API that browser_ext_sessions_getRecentlyClosed_tabs.js tests might be broken
Categories
(WebExtensions :: Untriaged, enhancement, P3)
WebExtensions
Untriaged
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mossop, Assigned: bsilverberg)
References
Details
(Whiteboard: [sessions]triaged)
Attachments
(1 file)
Over in bug 1337627 I was hitting problems with browser_ext_sessions_getRecentlyClosed_tabs.js loading about:addons and then closing it without waiting for initialization to complete so I was switching it to a different page. A few other pages I tried found errors: about:buildconfig made the test fail with an unexpected title for the page (Got null, expected about:buildconfig). http://example.com/ made the test fail with an unexpected favicon (Got http://example.com/favicon.ico, expected null). I didn't see anything obviously broken in the test but didn't want to dig too deep so I just found another page that works to unblock me, but my suspicion is that the API that this is testing might be broken in some subtle way.
Assignee | ||
Comment 1•7 years ago
|
||
Thanks for reporting this, Dave. I will look into it.
Assignee: nobody → bob.silverberg
Whiteboard: investigate
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: investigate → [sessions]triaged
Assignee | ||
Comment 2•7 years ago
|
||
I have looked into this and there are two problems here. 1. The test is not waiting long enough for the favicon to resolve before storing the information to be used later in an assertion. This didn't matter with the url currently being used in the test, but does manifest when using http://example.com/. I will fix this in the test and use that url to prove it is fixed. 2. There is an issue with SessionStore that manifests when a page has a title that is the same as the url (e.g., about:buildconfig). The title is not stored by SessionHistory, which means that when the data is returned by SessionStore the title is missing. This _could_ be fixed in the sessions API by using that assumption to populate the title, but I would rather fix this in SessionStore. I am currently discussing the fix for that with Mike deBoer and will open a separate bug for that once we decide what to do about it.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
I've added a patch to this bug which fixes the timing problem in the test. I'm going to hold off requesting review of that until bug 1344857 lands though, as the test also depends on that now.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8857078 [details] Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title, https://reviewboard.mozilla.org/r/128976/#review132598
Attachment #8857078 -
Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Hrm, adding to the timeout...do you know what part is taking that long?
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 12•7 years ago
|
||
It's the whole damn test! I already had to add that to browser_ext_sessions_getRecentlyClosed.js [1], and I think my change to the test that is making it wait a bit longer is what has caused this to go intermittent on try with a timeout. It's only on certain platforms and debug builds, and I think all the opening and closing of windows is just really slow there. If you look at the test you'll see it isn't actually doing a lot of different things, so there's no logical place to split it up. If you have any suggestions I'd be happy to try, but barring that I'm not sure what else to do than request a longer timeout. [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed.js#7
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 13•7 years ago
|
||
Actually, looking more closely at the try run, I think something else is going on. None of the test seems to be running before the timeout, so maybe there's a problem with the new code I added which opens the first tab. I'll look into that further next week.
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8857078 [details] Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title, https://reviewboard.mozilla.org/r/128976/#review132672 ::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:77 (Diff revision 6) > checkTabInfo(expectedTabs[x], tabInfos[x]); > } > > await extension.unload(); > > // Test without tabs permission. This should probably be in its own add_task
Comment 15•7 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #13) > Actually, looking more closely at the try run, I think something else is > going on. None of the test seems to be running before the timeout, so maybe > there's a problem with the new code I added which opens the first tab. I'll > look into that further next week. Yeah, I took a look at the logs and saw a bunch of stuff that seems potentially unrelated, it could be a prior test causing issues.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8857078 [details] Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title, https://reviewboard.mozilla.org/r/128976/#review132672 > This should probably be in its own add_task I'm not sure that makes sense to do. The tests here rely on all of the tabs that have been opened and closed above, as well as the data we stored for those tabs - basically the code from line 39 - 51 in the test, so just moving this code into its own task won't work. I could break it into two tasks, and do all the work from lines 39 - 51 in each task, but considering that that code is what is causing the test to take so long to run, it seems like that would create more problems than it solves. Am I misunderstanding your suggestion, or does what I'm saying make sense?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
I've spent a lot of time trying to get this test passing consistently on try, when using http://example.com as the first tab. There are issues with waiting for BrowserTestUtils.browserStopped, which seems to work locally for me, but fails pretty much everywhere on try. I believe the issue is also caused by a quirk with favicon.ico which causes a default one to be reported when there is not actually a favicon.ico defined for the page, which then causes my assertion to fail. I only added this to the test because this bug originally talked about how the test started to fail when trying to use http://example.com/, so I thought it worthwhile to fix that case, but that's really just an issue with the test, and there is no requirement to run this test with that URL loaded into a tab. The test is perfectly valid with only about: URLs loaded, so I don't think it's worth any more of my time to try to solve this problem when it doesn't actually make the test any better. I did update the test to use about:buildconfig as that did uncover an issue which I have since fixed. Shane, what do you think? Also, you never replied to my question in comment 16, although I think it's now moot.
Flags: needinfo?(mixedpuppy)
Comment 21•7 years ago
|
||
If that fixes it, cool, r=me OP suggested that using about:buildconfig failed the test, is that an issue?
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #21) > If that fixes it, cool, r=me OP suggested that using about:buildconfig > failed the test, is that an issue? No, that's what I fixed via bug 1344857, and I've updated this test to use about:buildconfig so we have coverage for that.
Comment 23•7 years ago
|
||
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2e1139a478b4 Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title, r=mixedpuppy
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e1139a478b4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•