Closed Bug 805452 Opened 9 years ago Closed 9 years ago

Write Marionette tests for battery charging status changes

Categories

(Core :: DOM: Device Interfaces, defect)

19 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 805838
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
The replacement battery level tests will follow in bug 805448
Attachment #686048 - Flags: review?(jgriffin)
Attachment #686048 - Flags: review?(jgriffin) → review?(rwood)
Attachment #686048 - Flags: feedback?(mounir)
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 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)
(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.
(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!
(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.
Updated with review feedback.
Attachment #686048 - Attachment is obsolete: true
Attachment #689826 - Flags: review?(rwood)
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+
Thanks Rob, marking as checkin-needed.
Keywords: checkin-needed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
https://hg.mozilla.org/mozilla-central/rev/25e87c4b7645
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.