1138426 - convert accessibility setIntPref to pushPrefEnv

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -0800)]

https://dxr.mozilla.org/mozilla-central/search?limit=100&redirect=false&q=setIntPref%20path%3Aaccessible%2F%20path%3Aaccessible/tests%2F

A lot of work has been done in bug 1056851 to convert setBoolPref's, so please look at the patches attached there for ideas on how to convert tests and use the pushPrefEnv properly :)

Comment 1

[Martijn Wargers (dead)]

The use of pushPrefEnv guarantees that the preferences are reset into their original state after the mochitest has completed.
Joel, is there any other reason why we want to convert chrome/browser-chrome tests?
I thought that nowadays, there is some check that guarantees that preferences are in their initial state after each mochitest.

Comment 2

[Joel Maher ( :jmaher ) (UTC -0800)]

I believe this is still needed.  I cannot remember or quickly find any code that checks preferences between tests.  That was the intention of pushPrefEnv.

Comment 3

[Martijn Wargers (dead)]

Ah, right, there is a patch in bug 995463, but that never landed (because it's not ready).
I can take this.

Comment 4

[Martijn Wargers (dead)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d26d28d325b

Comment 5

[Martijn Wargers (dead)]

Attachment: 1138426_accessible.diff

Yura, ok if I ask you review on this patch?

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8728503 [details] [diff] [review]
1138426_accessible.diff

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

Looks pretty good, i have some suggestions regarding the async output tests. Thanks!

::: accessible/tests/mochitest/jsat/test_alive.html
@@ +17,5 @@
>        AccessFuTest.once_log("AccessFu:Enabled", () =>
>          ok(AccessFu._enabled, "AccessFu was enabled again."));
>        AccessFuTest.once_log("EventManager.start", AccessFuTest.nextTest);
>        // Start AccessFu via pref.
> +      SpecialPowers.pushPrefEnv({"set": [['accessibility.accessfu.activate', 1]]}, function() {});

nit here and below, no need for second argument.

::: accessible/tests/mochitest/jsat/test_content_text.html
@@ +185,5 @@
>             new ExpectedCursorChange([{string: 'entry'}]),
>             new ExpectedTextSelectionChanged(0, 0)
>            ],
>            [function() {
> +             SpecialPowers.pushPrefEnv({"set": [[KEYBOARD_ECHO_SETTING, 3]]}, function() {

nit here and below, can we have second argument look like:
typeKey(...) rather than
function() {
  typeKey(...)();
}

::: accessible/tests/mochitest/jsat/test_landmarks.html
@@ +115,5 @@
>            "a child complementary"], ["a child complementary",
>            {"string": "complementary"}]]
>        }];
>  
> +

nit: whitespace

@@ +120,1 @@
>        // Test outputs (utterance and braille) for landmarks.

If you import jsatcommon.js file you can use its async machinery: so this test can look like
(notice I flipped from iterating output order before tests so we only set the pref twice):

      function testOutputOrder(aOutputOrder) {
        return function() {
          SpecialPowers.pushPrefEnv({
            "set": [[PREF_UTTERANCE_ORDER, aOutputOrder]]
          }, function() {
            tests.forEach(function run(test) {
              testOutput(test.expectedUtterance[aOutputOrder], test.accOrElmOrID,
                test.oldAccOrElmOrID, 1);
              testOutput(test.expectedBraille[aOutputOrder], test.accOrElmOrID,
                test.oldAccOrElmOrID, 0);
            });
            AccessFuTest.nextTest();
          });
        };
      }

      AccessFuTest.addFunc(testOutputOrder(0));
      AccessFuTest.addFunc(testOutputOrder(1));
      AccessFuTest.waitForExplicitFinish();
      AccessFuTest.runTests();


I would also structure the rest of the tests where we are adding these nested callbacks to look this way.

::: accessible/tests/mochitest/jsat/test_live_regions.html
@@ +13,5 @@
>            src="./jsatcommon.js"></script>
>    <script type="application/javascript">
>  
>      function startAccessFu() {
> +      SpecialPowers.pushPrefEnv({"set": [['accessibility.accessfu.activate', 1]]}, function() {});

nit: same here and below, no need for second argument afaik

::: accessible/tests/mochitest/jsat/test_quicknav_modes.html
@@ +14,5 @@
>    <script type="application/javascript">
>  
>      function prefStart() {
>        // Start AccessFu via pref.
> +      SpecialPowers.pushPrefEnv({"set": [['accessibility.accessfu.activate', 1]]}, function() {});

same here and below, no need for second argument.

@@ +42,5 @@
> +             SpecialPowers.pushPrefEnv(
> +               {"set": [['accessibility.accessfu.quicknav_index', aModeIndex]]},
> +               function() {
> +                 _expectMode(aExpectedMode, AccessFuTest.nextTest);
> +             });

nit: indentation.

@@ +53,5 @@
> +        }, function() {
> +          // When the modes are reconfigured, the current mode should
> +          // be set to the first in the new list.
> +          _expectMode('Landmark', AccessFuTest.nextTest);
> +        });

nit: indentation

Comment 7

[Martijn Wargers (dead)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b01fd6c5770b

Comment 8

[Martijn Wargers (dead)]

Attachment: 1138426_accessible.diff

Thanks for your review, I think I addressed your comments.

> If you import jsatcommon.js file you can use its async machinery: so this test can look like
> (notice I flipped from iterating output order before tests so we only set the pref twice):

Thanks for that, I don't really understand this thing myself, but it seems to work.

One question:
If I run this test for example: ./mach mochitest a11y accessible/tests/mochitest/jsat/test_tables.html
It also runs this test afterwards:
 browser/base/content/test/general/browser_autocomplete_a11y_label.js
Why is that?

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8737195 [details] [diff] [review]
1138426_accessible.diff

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

Thanks this looks good!

Comment 10

[Martijn Wargers (dead)]

Attachment: 1138426_accessible.diff (for check-in)

Comment 11

[Martijn Wargers (dead)]

Thanks for the review Yura, but could you answer the question I have in comment 8, if you know the answer?

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #11)
> Thanks for the review Yura, but could you answer the question I have in
> comment 8, if you know the answer?

Ah sorry, yes so running it locally on OSX doesn't run the test you mentioned. Are there specific steps that trigger it for you ?

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

^ For me at least

Comment 14

[Martijn Wargers (dead)]

(In reply to Yura Zenevich [:yzen] from comment #12)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #11)
> > Thanks for the review Yura, but could you answer the question I have in
> > comment 8, if you know the answer?
> 
> Ah sorry, yes so running it locally on OSX doesn't run the test you
> mentioned. Are there specific steps that trigger it for you ?

No, I have a clean debug tree, just running ./mach mochitest a11y accessible/tests/mochitest/jsat/test_tables.html does it for me.
Anyway, I filed bug 1261884 for it now.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ec31248db6

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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