Closed Bug 1138426 Opened 6 years ago Closed 5 years ago

convert accessibility setIntPref to pushPrefEnv

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jmaher, Assigned: martijn.martijn)

Details

Attachments

(1 file, 2 obsolete files)

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 :)
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.
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
Ah, right, there is a patch in bug 995463, but that never landed (because it's not ready).
I can take this.
Assignee: nobody → martijn.martijn
Attached patch 1138426_accessible.diff (obsolete) — Splinter Review
Yura, ok if I ask you review on this patch?
Attachment #8728503 - Flags: review?(yzenevich)
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
Attachment #8728503 - Flags: review?(yzenevich)
Attached patch 1138426_accessible.diff (obsolete) — Splinter Review
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?
Attachment #8728503 - Attachment is obsolete: true
Attachment #8737195 - Flags: review?(yzenevich)
Comment on attachment 8737195 [details] [diff] [review]
1138426_accessible.diff

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

Thanks this looks good!
Attachment #8737195 - Flags: review?(yzenevich) → review+
Attachment #8737195 - Attachment is obsolete: true
Keywords: checkin-needed
Thanks for the review Yura, but could you answer the question I have in comment 8, if you know the answer?
Flags: needinfo?(yzenevich)
(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 ?
Flags: needinfo?(yzenevich)
(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.
https://hg.mozilla.org/mozilla-central/rev/a6ec31248db6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.