Closed
Bug 1138426
Opened 6 years ago
Closed 5 years ago
convert accessibility setIntPref to pushPrefEnv
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jmaher, Assigned: martijn.martijn)
Details
Attachments
(1 file, 2 obsolete files)
22.92 KB,
patch
|
Details | Diff | Splinter Review |
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 :)
Assignee | ||
Comment 1•5 years ago
|
||
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)
Reporter | ||
Comment 2•5 years ago
|
||
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)
Assignee | ||
Comment 3•5 years ago
|
||
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
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d26d28d325b
Assignee | ||
Comment 5•5 years ago
|
||
Yura, ok if I ask you review on this patch?
Attachment #8728503 -
Flags: review?(yzenevich)
Comment 6•5 years ago
|
||
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)
Assignee | ||
Comment 7•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b01fd6c5770b
Assignee | ||
Comment 8•5 years ago
|
||
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 9•5 years ago
|
||
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+
Assignee | ||
Comment 10•5 years ago
|
||
Attachment #8737195 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•5 years ago
|
||
Thanks for the review Yura, but could you answer the question I have in comment 8, if you know the answer?
Flags: needinfo?(yzenevich)
Comment 12•5 years ago
|
||
(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)
Comment 13•5 years ago
|
||
^ For me at least
Assignee | ||
Comment 14•5 years ago
|
||
(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•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ec31248db6
Keywords: checkin-needed
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a6ec31248db6
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•