Closed Bug 1113199 Opened 5 years ago Closed 5 years ago

Create tests for bug 1110872

Categories

(Core :: DOM: Device Interfaces, defect)

25 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: qdot, Assigned: qdot)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

Bug 1110872 had to land before tests were verified due to dogfooding needs. This bug is the followup for fixing/landing tests.
Latest version of tests. Things I still need to do:

- Create many more locks and and file many more queries to make sure I don't exhaust all queries before reload.
- See if I can simplify test reloading. May be able to use get variables instead of multi-file iframe setup.
Tried to make test more stable by opened a BUNCH of locks before closing iframe, added e10s only attribute for running test.
Attachment #8538581 - Attachment is obsolete: true
Comment on attachment 8540910 [details] [diff] [review]
Patch 1 (v4) - Mochitest for testing Settings OOP Reload Error

Try is looking better now.
Attachment #8540910 - Flags: review?(lissyx+mozillians)
Comment on attachment 8540910 [details] [diff] [review]
Patch 1 (v4) - Mochitest for testing Settings OOP Reload Error

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

Just a few nits/comments on how this works. I forced a set of 200 retriggers on try, that loos 100% rock stable on opt and debug builds

Not r+ yet, because I want to play with it on my laptop and I'll do this later today :).

::: dom/settings/tests/file_bug1110872.html
@@ +8,5 @@
> +         SpecialPowers.addPermission("settings-read", true, document);
> +         SpecialPowers.addPermission("settings-write", true, document);
> +         SpecialPowers.addPermission("settings-api-read", true, document);
> +         SpecialPowers.addPermission("settings-api-write", true, document);
> +         SpecialPowers.addPermission("settings-clear", true, document);

nit: Given that we only do lock.get(), do we really want to set all those permissions?

@@ +27,5 @@
> +         window.onload = function() {
> +             window.addEventListener("message", function (msg) {
> +                 var i;
> +                 var reqs = [];
> +                 if (msg.data.step == 1) {

So we send a batch of 100 createLock().get(), and then we force kill with .reload().
Then at the next load, we just create one lock and do a get on it. That looks fine
to me.

::: dom/settings/tests/mochitest.ini
@@ +7,2 @@
>  
> +# [test_settings_basics.html]

I get that you disabled this just for your try, do not forget to re-enable :)
Kyle, just for the sake of completeness, would you mind triggering a set of try builds with lots of xpcshell test retrigger, and with the fix of bug 1110872 disabled ?

This way we could expose on the try results that it does properly test the fix :)
Flags: needinfo?(kyle)
Why? This isn't an xpcshell test.
Flags: needinfo?(kyle)
Yeah, sorry, I meant mochitest instead of xpcshell :(
Flags: needinfo?(kyle)
Flags: needinfo?(kyle)
Try with all the Settings tests enabled, and the fix for bug 1110872.

https://tbpl.mozilla.org/?tree=Try&rev=974fb318f93a
Try with all the Settings tests enabled, and without the fix for bug 1110872.

https://tbpl.mozilla.org/?tree=Try&rev=279b1366391c
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Try with all the Settings tests enabled, and without the fix for bug 1110872.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=279b1366391c

Orange on 200 retriggers for both opt and debug m-e10s
(In reply to Alexandre LISSY :gerard-majax from comment #9)
> Try with all the Settings tests enabled, and the fix for bug 1110872.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=974fb318f93a

Green on 200 retriggers for both opt and debug m-e10s, just a couple of known unrelated orange on debug.
Comment on attachment 8540910 [details] [diff] [review]
Patch 1 (v4) - Mochitest for testing Settings OOP Reload Error

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

Very good, let's land this :)
Attachment #8540910 - Flags: review?(lissyx+mozillians) → review+
https://hg.mozilla.org/mozilla-central/rev/ebab1280e934
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Whiteboard: [systemsfe]
You need to log in before you can comment on or make changes to this bug.