Closed Bug 1325099 Opened 3 years ago Closed 3 years ago

toolkit/modules/tests/browser/browser_Battery.js fails to run more than once in same browser session

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: malayaleecoder, Assigned: malayaleecoder)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

./mach mochitest toolkit/modules/tests/browser/browser_Battery.js  --repeat 1 fails.
Blocks: 1316113
Attached patch Bug1325099_v1.diff (obsolete) — Splinter Review
Not sure whether this change will break the purpose of the test. Do tell if so.
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Attachment #8820787 - Flags: review?(jmaher)
Component: Session Restore → General
Product: Firefox → Toolkit
Comment on attachment 8820787 [details] [diff] [review]
Bug1325099_v1.diff

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

Chris, I see in bug 1259335 that you removed most of the code related to this test, and I don't see where we added it back.  Can you comment on if this test is useful and if the change here breaks the intention of the test?

::: toolkit/modules/tests/browser/browser_Battery.js
@@ +12,5 @@
>    GetBattery().then(function (battery) {
>      for (let k of ["charging", "chargingTime", "dischargingTime", "level"]) {
>        let backup = battery[k];
>        battery[k] = "__magic__";
> +      is(battery[k], backup, "Setting battery " + k + " preference without spoofing enabled should fail");

I am not seeing the change on this line- is it necessary?
Attachment #8820787 - Flags: review?(jmaher)
Attachment #8820787 - Flags: review?(cpeterson)
Attachment #8820787 - Flags: review+
Comment on attachment 8820787 [details] [diff] [review]
Bug1325099_v1.diff

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

(In reply to Joel Maher ( :jmaher) from comment #3)
> Chris, I see in bug 1259335 that you removed most of the code related to
> this test, and I don't see where we added it back.  Can you comment on if
> this test is useful and if the change here breaks the intention of the test?

I removed the nonstandard navigator.battery API in favor of the standard promise-based navigator.getBattery() API. So there is no navigator.battery API to test. :) The tests in the GetBattery().then() function call are still valid.

I don't know what imported.Debugging.fake does, so I can't say whether this change is appropriate or not. <:)


> > +      is(battery[k], backup, "Setting battery " + k + " preference without spoofing enabled should fail");
> 
> I am not seeing the change on this line- is it necessary?

There is now a space printed between the k value and the word "preference".

::: toolkit/modules/tests/browser/browser_Battery.js
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Vishnu, you might consider adding "use strict"; here so this test runs in JavaScript strict mode to help find test bugs. Whenever I modify a test, I like to slip in a "use strict"; statement if there wasn't one already. :)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode
Attachment #8820787 - Flags: review?(cpeterson) → feedback+
Attached patch Bug1325099_v2.diff (obsolete) — Splinter Review
Thanks for the suggestion, Chris.
@jmaher , I could not push to try due to slow internet. Do have a look at this patch and if possible, can you push to try for me?
Cheers!
Attachment #8820787 - Attachment is obsolete: true
Attachment #8821278 - Flags: review?(jmaher)
Comment on attachment 8821278 [details] [diff] [review]
Bug1325099_v2.diff

thanks Vishnu for working on this.  One thing we should ensure we do is in the catch ignore the error if it is a TypeError, otherwise raise the exception so we don't miss anything.
Attachment #8821278 - Flags: review?(jmaher)
Done :D
Attachment #8821278 - Attachment is obsolete: true
Attachment #8821630 - Flags: review?(jmaher)
Comment on attachment 8821630 [details] [diff] [review]
Bug1325099_v3.diff

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

I will land when i see the try run completed!
Attachment #8821630 - Flags: review?(jmaher) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce813a77054e
reset imported.Debugging.fake value and catch TypeError. r=jmaher
https://hg.mozilla.org/mozilla-central/rev/ce813a77054e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.