Closed Bug 1080801 Opened 10 years ago Closed 10 years ago

Breakdown: Investigate tests in browser/base/content/test/general disabled in e10s

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Iteration:
36.3
Tracking Flags:
Tracking Status
e10s + ---

People

(Reporter: mossop, Assigned: Gijs)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

update e10s skipping for browser/base tests, step 1 of N: zoom
7.48 KB, patch
ally
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
enable tests relying on DOMLinkAdded,
3.49 KB, patch
ally
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
re-enable popupnotification tests that actually work in e10s,
4.76 KB, patch
mconley
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
about:newtab session history stuff works in e10s now so enable the test,
1.65 KB, patch
mossop
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
re-enable test waiting for bug 930863 which is already fixed,
1.34 KB, patch
ally
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
re-enable the working urlbar tests,
3.42 KB, patch
Gavin
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
fix browser_bug633691.js to work in e10s mode,
2.65 KB, patch
MattN
: review+
Details | Diff | Splinter Review
There are 136 tests disabled in e10s mode in this manifest, that's over 10% of non-devtools frontend tests. Many claim to be depending on bugs that are already fixed. We need to figure out which ones can be enabled and make sure specific bugs are on file for those that can't
Flags: qe-verify-
Flags: firefox-backlog+
Points: --- → 5
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.2
Latest count: 144. So I'm just going to go through and either update bug links, file bugs, or re-enable, as much as I can... First off: there are 9 tests that are marked as depending on e10s zoom. However, e10s zoom is fixed, but the tests are disabled because of bug 1056146, it seems. They mostly still fail locally if I re-enable them. Here's a try push that re-enables one, which seems to work fine locally, and updates the comments for the other ones: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d4311bcf74bf
Depends on: 1056146
Ally, assuming the trypush checked out, can you review this?
Attachment #8516000 - Flags: review?(ally)
Depends on: 1093153
Keywords: leave-open
Depends on: 1093155
(In reply to :Gijs Kruitbosch from comment #1) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d4311bcf74bf These were green; retriggered bc1 two more times on all platforms to make sure that test isn't going to be an intermittent... In other news, I'm tracking this in a spreadsheet because I think otherwise I might go crazy: https://docs.google.com/spreadsheets/d/1hLBi9xSvbO5HeZfS2fYU8FtpoGf0P7mYryFCOz9h2o8/edit?usp=sharing I... can't say I've fully worked out how to sensibly keep this up-to-date yet, so I'm just marking rows as 'done' instead, so I can filter appropriately. (while I could rerun the trivial grep that I used to make the list and re-paste it in the first sheet, when N rows go missing, any additional annotations I made will be off-by-N)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=825b8dfd8c95 Bug 918663 was fixed a while back (or rather, bug 961529), and so all of these pass for me locally.
Attachment #8516068 - Flags: review?(ally)
Depends on: 1093206
(In reply to :Gijs Kruitbosch from comment #3) > (In reply to :Gijs Kruitbosch from comment #1) > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d4311bcf74bf > > These were green; retriggered bc1 two more times on all platforms to make > sure that test isn't going to be an intermittent... What was the result of re-running bc1? Did it stay orange?
Comment on attachment 8516068 [details] [diff] [review] enable tests relying on DOMLinkAdded, Review of attachment 8516068 [details] [diff] [review]: ----------------------------------------------------------------- looks pretty okay. Do you know what work was done to make these e10s friendly now?
Attachment #8516068 - Flags: review?(ally) → review+
(In reply to please NEEDINFO? :ally Allison Naaktgeboren from comment #8) > (In reply to :Gijs Kruitbosch from comment #3) > > (In reply to :Gijs Kruitbosch from comment #1) > > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d4311bcf74bf > > > > These were green; retriggered bc1 two more times on all platforms to make > > sure that test isn't going to be an intermittent... > > What was the result of re-running bc1? Did it stay orange? The 10.6 orange appeared in the retrigger; it's an unrelated intermittent failure in session store, as best I can tell. Otherwise all the retriggers were green.
(In reply to please NEEDINFO? :ally Allison Naaktgeboren from comment #9) > Comment on attachment 8516068 [details] [diff] [review] > enable tests relying on DOMLinkAdded, > > Review of attachment 8516068 [details] [diff] [review]: > ----------------------------------------------------------------- > > looks pretty okay. Do you know what work was done to make these e10s > friendly now? bug 961529, which the bug referenced in the comments (removed by this patch) was duped to, was fixed. The notifications are now properly propagated from content to the parent process, AFAIK.
Depends on: 866413
Comment on attachment 8516000 [details] [diff] [review] update e10s skipping for browser/base tests, step 1 of N: zoom Review of attachment 8516000 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/question. I'd like to know what you did that caused this test to start working ::: browser/base/content/test/general/browser.ini @@ -210,5 @@ > [browser_bug555767.js] > skip-if = e10s # Bug 916974 - Session history doesn't work in e10s > [browser_bug556061.js] > [browser_bug559991.js] > -skip-if = e10s # Bug 691614 - no e10s zoom support yet test browser_bug561623.js works now? Does this one not use the promiseTabLoadEvent? Other than this, this patch is all updated comments
Attachment #8516000 - Flags: review?(ally) → review+
(In reply to please NEEDINFO? :ally Allison Naaktgeboren from comment #12) > Comment on attachment 8516000 [details] [diff] [review] > update e10s skipping for browser/base tests, step 1 of N: zoom > > Review of attachment 8516000 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ w/question. I'd like to know what you did that caused this test to start > working > > ::: browser/base/content/test/general/browser.ini > @@ -210,5 @@ > > [browser_bug555767.js] > > skip-if = e10s # Bug 916974 - Session history doesn't work in e10s > > [browser_bug556061.js] > > [browser_bug559991.js] > > -skip-if = e10s # Bug 691614 - no e10s zoom support yet > > test browser_bug561623.js works now? No, skip-if lines apply to the [test] *above* the skip-if instruction. Think ini files with headers over sections. So browser_bug559991.js works now that zoom works on e10s. > Does this one not use the promiseTabLoadEvent? It does, via FullZoomHelper.load (in browser/base/content/test/general/head.js). However, promiseTabLoadEvent isn't actually broken in e10s; It seems the rest of the fullzoom (test helper) code is busted somehow for the other tests (all the other tests whose comments I changed fail locally). I guess I'll morph bug 1056146 to reflect that and update these comments. > Other than this, this patch is all updated comments Yeah; I guess next time I'll just land that part without r?; it doesn't really make sense to bother anyone else with that. Sorry!
Depends on: 1093373
Depends on: 1093404
Attachment #8516000 - Flags: checkin+
Attachment #8516068 - Flags: checkin+
remote: https://hg.mozilla.org/integration/fx-team/rev/5f3863fb4bb4 According to my spreadsheet, this marks 36/144, ie 25% done... :-\
These tests seem to work. The urlbar single word one is my own fault, it was fixed in bug 1048618. I expect the disabled popup transitions is what makes the other test work.
Attachment #8516646 - Flags: review?(mconley)
Depends on: 1093586
Depends on: 1093594
Depends on: 653065
Depends on: 1093603
Depends on: 1093607
Depends on: 1093642
Depends on: 1071623
Attachment #8516646 - Attachment is obsolete: true
Attachment #8516646 - Flags: review?(mconley)
Depends on: 1093677
Comment on attachment 8516747 [details] [diff] [review] about:newtab session history stuff works in e10s now so enable the test, Review of attachment 8516747 [details] [diff] [review]: ----------------------------------------------------------------- Oops forgot to re-enable that. Thanks
Attachment #8516747 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8516735 [details] [diff] [review] re-enable popupnotification tests that actually work in e10s, Review of attachment 8516735 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8516735 - Flags: review?(mconley) → review+
(In reply to :Gijs Kruitbosch from comment #20) > https://hg.mozilla.org/integration/fx-team/rev/a51796d31cf4 I went ahead and fixed the bustage this caused for you. https://hg.mozilla.org/integration/fx-team/rev/211787a9b23a
Depends on: 1093756
Attachment #8516747 - Flags: checkin+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > (In reply to :Gijs Kruitbosch from comment #20) > > https://hg.mozilla.org/integration/fx-team/rev/a51796d31cf4 > > I went ahead and fixed the bustage this caused for you. > https://hg.mozilla.org/integration/fx-team/rev/211787a9b23a As noted on IRC, sorry. :-( Please come find me in portland for a beverage or two of your choice.
Ally, seems like you fixed most of this in bug 930863 and this test now works (at least locally; try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3105800ab6ff ).
Attachment #8517015 - Flags: review?(ally)
Depends on: 940206
Depends on: 1069757
Comment on attachment 8517015 [details] [diff] [review] re-enable test waiting for bug 930863 which is already fixed, Review of attachment 8517015 [details] [diff] [review]: ----------------------------------------------------------------- oh yeah, one of my very first e10s bugs! didn't even know it had a test. Assuming try remains remains green when it finishes, you should be good. tentative r+
Attachment #8517015 - Flags: review?(ally) → review+
Depends on: 1093941
Attachment #8517073 - Flags: review?(gavin.sharp) → review+
Depends on: 1093997
Comment on attachment 8517015 [details] [diff] [review] re-enable test waiting for bug 930863 which is already fixed, remote: https://hg.mozilla.org/integration/fx-team/rev/28de06bbcdda remote: https://hg.mozilla.org/integration/fx-team/rev/79d73c3b0b07 re-disabled the single word notification thing because it was intermittent on all those try pushes, as well as current fx-team: remote: https://hg.mozilla.org/integration/fx-team/rev/782f739ec63e followup for that in bug 1093997.
Attachment #8517015 - Flags: checkin+
Attachment #8517073 - Flags: checkin+
Depends on: 1094205
Depends on: 940195
Depends on: 1094222
Depends on: 1094240
Depends on: 863514
Depends on: 921957
Depends on: 1094252
Depends on: 1094510
(In reply to :Gijs Kruitbosch from comment #33) > Moar comments: > > remote: https://hg.mozilla.org/integration/fx-team/rev/fe77d671da45 > > And some more tests that seem to work: > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b60e6638f1c6 canonizeURL was very sad (bug 1094510). I found some other things that worked on my windows machine, rerunning with those (and canonizeURL re-disabled): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d095d99d129
Depends on: 1094761
(In reply to :Gijs Kruitbosch from comment #36) > (In reply to :Gijs Kruitbosch from comment #33) > > Moar comments: > > > > remote: https://hg.mozilla.org/integration/fx-team/rev/fe77d671da45 > > > > And some more tests that seem to work: > > > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b60e6638f1c6 > > canonizeURL was very sad (bug 1094510). I found some other things that > worked on my windows machine, rerunning with those (and canonizeURL > re-disabled): > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d095d99d129 Annnnd the next test to hit the network is browser_contextSearchTabPosition.js, which in a moment of impressive fail manages to crash the test(s) that run after it instead of itself. New try push: remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c32ab790f65c comments for canonizeURL and contextSearchTabPosition: remote: https://hg.mozilla.org/integration/fx-team/rev/5e34ddeb2042
https://hg.mozilla.org/integration/fx-team/rev/e19893ac6cb5 This concludes looking at all the tests that are specifically disabled on e10s (ie not just skip-if = true) that have a comment with a bug number. There are still some 40-odd tests other than the ones covered by bug 1094222 that need looking at. They split roughly equally into "says it's disabled because it touches content (without a bug number)" and "doesn't say that, but does have a comment, without a bug number". I plan to spend the rest of the iteration looking at those, and filing followup bugs for whatever I don't manage to look at before Tuesday.
Iteration: 36.2 → 36.3
Depends on: 1099154
Depends on: 1099156
Ran into this little blighter, seems trivial to just fix. Didn't see other tests that use this pattern that were as easy to fix, unfortunately.
Attachment #8524126 - Flags: review?(mconley)
Depends on: 1100653
Depends on: 1100662
Depends on: 1100664
Depends on: 1100687
Depends on: 1100698
Depends on: 933103
Depends on: 1100700
Depends on: 1100703
Depends on: 1100707
More comments: https://hg.mozilla.org/integration/fx-team/rev/022cf9a01355 These seem to work locally, so trypushing: remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d8405115ae3 those plus the patch for 633691 conclude looking all the bugless things that say they touch content.
(In reply to :Gijs Kruitbosch from comment #45) > These seem to work locally, so trypushing: > > remote: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3d8405115ae3 Green post-retrigger-pocalypse, so: https://hg.mozilla.org/integration/fx-team/rev/b288633302f7
Depends on: 1101973
Depends on: 1101993
Depends on: 1102015
Depends on: 1102017
Depends on: 1102018
Depends on: 1102020
Depends on: 1102025
(In reply to :Gijs Kruitbosch from comment #50) > ... and > > remote: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eed5473256af > > for more enabling-checking. Surprisingly, all-green: https://hg.mozilla.org/integration/fx-team/rev/2df1080da369
Not counting the test for which I'm waiting for review, this leaves 24 tests which need to be examined, which are the subject of bug 1094205 as they have 0 comments as to why they are e10s-disabled. All the other tests now have up-to-date bug numbers and as best an explanation I could find as to why they're disabled. I intend to remove leave-open after the previous cset lands on m-c, and close this bug once I can land the aforementioned patch.
Attachment #8524126 - Flags: review?(mconley) → review+
Comment on attachment 8524126 [details] [diff] [review] fix browser_bug633691.js to work in e10s mode, >+ gBrowser.selectedTab.linkedBrowser.addEventListener("load", testIframeCert, true); gBrowser.selectedBrowser
remote: https://hg.mozilla.org/integration/fx-team/rev/20d336286e35 Dao's comment dropped in my inbox while I was already pushing, so sadly, followup commit: remote: https://hg.mozilla.org/integration/fx-team/rev/27f47124cb27
Keywords: leave-open
Depends on: 1102331
Had to re-disable another test because it basically only runs "meaningfully" on 10.8, which isn't run on try by default because of capacity issues, apparently... - and of course, it broke when I actually landed the enabling patch. https://hg.mozilla.org/integration/fx-team/rev/e667086cc663
Depends on: 1251622
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: