Closed
Bug 805452
Opened 12 years ago
Closed 12 years ago
Write Marionette tests for battery charging status changes
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: davehunt, Assigned: davehunt)
References
Details
Attachments
(1 file, 1 obsolete file)
17.12 KB,
patch
|
rwood
:
review+
|
Details | Diff | Splinter Review |
We need automated tests for the Battery API to ensure that simulated changes to the battery charging status (charging, discharging, full, not-charging, unknown) fire the appropriate events. As these tests will require simulating hardware changes, they will be necessary to run in the emulator and will therefore use Marionette.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dave.hunt
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
The replacement battery level tests will follow in bug 805448
Attachment #686048 -
Flags: review?(jgriffin)
Assignee | ||
Updated•12 years ago
|
Attachment #686048 -
Flags: review?(jgriffin) → review?(rwood)
Assignee | ||
Updated•12 years ago
|
Attachment #686048 -
Flags: feedback?(mounir)
Comment 2•12 years ago
|
||
Comment on attachment 686048 [details] [diff] [review] Replaces the existing battery test with a set of more complete status tests. v1.0 Review of attachment 686048 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, and all the tests passed for me locally. Just one suggestion and one nit: The event handler isn't receiving the event itself. battery.onchargingchange = function () { It wouldn't hurt to verify the actual event i.e.: battery.onchargingchange = function (event) { is(event.type, "chargingchange", "event type"); Same comment applies for the event handler in all of the tests. Also just a nit: I have started using the optional parameters in the is() and ok() calls to describe what is being verified, as I find this really helps when debugging tests in adb logcat. i.e.: is(battery.charging, toCharging, "battery.charging");
Attachment #686048 -
Flags: review?(rwood) → review-
Comment 3•12 years ago
|
||
Comment on attachment 686048 [details] [diff] [review] Replaces the existing battery test with a set of more complete status tests. v1.0 Review of attachment 686048 [details] [diff] [review]: ----------------------------------------------------------------- I do not really understand those tests TBH. I'm not really used to marionnette tests, it might be why ;) One comment that I would like to say is that you do not seem to be testing .level, .remainingChargingTime and .remainingDischargingTime. .level is easy to test: when full, it is 1.0, otherwise, it it in ]0.0, 1.0[ .remainingCharging time is 0.0 when full and charging, another value otherwise .remainingDischarging time is Infinity when full and charging, another value otherwise The two last property might have different values that the one listed there in B2G because they are not implemented correctly.
Attachment #686048 -
Flags: feedback?(mounir)
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Rob Wood [:rwood] from comment #2) > The event handler isn't receiving the event itself. > > battery.onchargingchange = function () { > > It wouldn't hurt to verify the actual event i.e.: > > battery.onchargingchange = function (event) { > is(event.type, "chargingchange", "event type"); Good point. I can add this in an updated patch. > I have started using the optional parameters in the is() and ok() calls to > describe what is being verified, as I find this really helps when debugging > tests in adb logcat. i.e.: > > is(battery.charging, toCharging, "battery.charging"); Also a good suggestion, thanks.
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #3) > One comment that I would like to say is that you do not seem to be testing > .level, .remainingChargingTime and .remainingDischargingTime. Level will be tested in the upcoming patch for bug 805448 The remaining charging/discharging time is always infinity for the emulator, which these tests will run against and there's no way to test this here. Thanks for the feedback!
Comment 6•12 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #5) > (In reply to Mounir Lamouri (:mounir) from comment #3) > > One comment that I would like to say is that you do not seem to be testing > > .level, .remainingChargingTime and .remainingDischargingTime. > > Level will be tested in the upcoming patch for bug 805448 > > The remaining charging/discharging time is always infinity for the emulator, > which these tests will run against and there's no way to test this here. > > Thanks for the feedback! You could write tests using todo() using the specification to know what is the expected result but I would understand it's a fair amount of work for no short term outcome.
Assignee | ||
Comment 7•12 years ago
|
||
Updated with review feedback.
Attachment #686048 -
Attachment is obsolete: true
Attachment #689826 -
Flags: review?(rwood)
Comment 8•12 years ago
|
||
Comment on attachment 689826 [details] [diff] [review] Replaces the existing battery test with a set of more complete status tests. v1.1 Review of attachment 689826 [details] [diff] [review]: ----------------------------------------------------------------- Lgtm, and all of the tests pass on my VM.
Attachment #689826 -
Flags: review?(rwood) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Thanks Rob, marking as checkin-needed.
Keywords: checkin-needed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e87c4b7645
Keywords: checkin-needed
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/25e87c4b7645
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 12•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef6e09eaf6e1 https://hg.mozilla.org/releases/mozilla-b2g18/rev/449d3d4c7b53
status-firefox19:
--- → fixed
status-firefox20:
--- → fixed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta] → [status-b2g18:fixed]
Updated•12 years ago
|
status-b2g18:
--- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in
before you can comment on or make changes to this bug.
Description
•