1113199 - Create tests for bug 1110872

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Bug 1110872 had to land before tests were verified due to dogfooding needs. This bug is the followup for fixing/landing tests.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - WIP: Mochitest for testing Settings OOP Reload Error

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.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Mochitest for testing Settings OOP Reload Error

Tried to make test more stable by opened a BUNCH of locks before closing iframe, added e10s only attribute for running test.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=734e96ef23c7
https://tbpl.mozilla.org/?tree=Try&rev=734e96ef23c7

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8540910 [details] [diff] [review]
Patch 1 (v4) - Mochitest for testing Settings OOP Reload Error

Try is looking better now.

Comment 5

[:gerard-majax]

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 :)

Comment 6

[:gerard-majax]

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 :)

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Why? This isn't an xpcshell test.

Comment 8

[:gerard-majax]

Yeah, sorry, I meant mochitest instead of xpcshell :(

Comment 9

[:gerard-majax]

Try with all the Settings tests enabled, and the fix for bug 1110872.

https://tbpl.mozilla.org/?tree=Try&rev=974fb318f93a

Comment 10

[:gerard-majax]

Try with all the Settings tests enabled, and without the fix for bug 1110872.

https://tbpl.mozilla.org/?tree=Try&rev=279b1366391c

Comment 11

[:gerard-majax]

(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

Comment 12

[:gerard-majax]

(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 13

[:gerard-majax]

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 :)

Comment 14

[:gerard-majax]

See try in comment 11 and comment 12

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ebab1280e934

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebab1280e934