If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert System to new Notification API: BatteryManager

RESOLVED DUPLICATE of bug 1095109

Status

Firefox OS
Gaia::System
RESOLVED DUPLICATE of bug 1095109
3 years ago
2 years ago

People

(Reporter: gerard, Assigned: rlustin, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe][good first bug])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

3 years ago
apps/system/js/battery_manager.js:
  - sending notification of power saving mode
(Reporter)

Updated

3 years ago
Whiteboard: [systemsfe][good first bug][mentor=gerard-majax]
Mentor: lissyx+mozillians@lissyx.dyndns.org
Whiteboard: [systemsfe][good first bug][mentor=gerard-majax] → [systemsfe][good first bug]
Hi!
I'm interested to fix this bug. Can somebody assigns it to me? Thanks!

Updated

3 years ago
Assignee: nobody → raphael
Created attachment 8444133 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax
Created attachment 8444136 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax
Attachment #8444133 - Attachment is obsolete: true
Comment on attachment 8444136 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax

Hi,
My first patch for B2G. Can you please take a look at it ?
Thanks :)
Attachment #8444136 - Flags: review?(lissyx+mozillians)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8444136 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax

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

Thanks for this patch. A few remarks, since you are new :)

First, I'm not a peer of the system app so I can't review this. You should ask someone who is a peer or owner of system component on https://wiki.mozilla.org/Modules/FirefoxOS#System
Also, since it's a gaia patch, you should rather open a pull request against gaia project on githb, and paste the link to the pull request in the attachment.

But I'll have a look at this anyway to give you a feedback :).

I think you need to add some unit tests to make sure the Notification API is properly called and its use won't regress. You are also lacking the notification close: previous API did not allowed to close a notification, but we now are able, so you can leverage this and make the notification go away as soon as the battery level is correct, for example. And we can probably cleanup the code removing the battery specific at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/battery_manager.js#L20

::: apps/system/js/battery_manager.js
@@ +265,5 @@
> +      _('notification-powersaving-mode-on-title'),
> +      {
> +        body: _('notification-powersaving-mode-on-description'),
> +        icon: 'style/icons/System.png',
> +        onclick: clickCB

It would be better to use addEventListener instead.

@@ +267,5 @@
> +        body: _('notification-powersaving-mode-on-description'),
> +        icon: 'style/icons/System.png',
> +        onclick: clickCB
> +      }
> +    );

Did you checked whether there was proper unit testing for this?
Attachment #8444136 - Flags: review?(lissyx+mozillians) → feedback+
(Reporter)

Comment 6

3 years ago
Alive, since you wrote quite a lot of this, you can probably give better review inputs.
Flags: needinfo?(alive)
The changes looks sane, but please make sure battery_manager_test passes.
Flags: needinfo?(alive)
Hi, please make a github pull request and have the CI run your patch.
Flags: needinfo?(raphael)
Hi,
Thanks for your tips.

Here is the link for CI Tests:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=4dcc29309782a596e7654eb7a661651ecd82f532

I'll have a look at EventListeners. I'll check if battery_manager_test passes.
Created attachment 8444166 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=alive

Use addEventListener instead of onclick.

TODO:
- Check if there's a proper unit test for this notification.
- Close the notification as soon as the battery level is correct.
- Cleanup the code removing the battery specific at
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/battery_manager.js#L20
Attachment #8444136 - Attachment is obsolete: true
battery_manager_test passes, but I don't now if power saving mode notification is tested.
Flags: needinfo?(raphael)
(In reply to Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel] from comment #11)
> battery_manager_test passes, but I don't now if power saving mode
> notification is tested.

Apparently there was no test for it before, so it'd be nice you created one.
(In reply to Alive Kuo [:alive][NEEDINFO!][OOO from 6/23 until 6/30] from comment #12)
> (In reply to Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel] from comment
> #11)
> > battery_manager_test passes, but I don't now if power saving mode
> > notification is tested.
> 
> Apparently there was no test for it before, so it'd be nice you created one.

voicemail_test has the some tips for you.
(Reporter)

Comment 14

3 years ago
Raphaël, are you still working on this?
Flags: needinfo?(raphael)
Hello Alexandre,

I'm working back on it, but I can't manage to execute unit tests.

I've builded firefox with latest sources, installed Node.js via package manager in order to install the very latest versions and delete the node_modules/test-agent folder, as described in https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

I've executed tests with `bin/gaia-test /home/xx/Documents/Dev/gaia /home/xx/Documents/Dev/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/bin/firefox`. Firefox starts, but it crashes with this cryptic error:


HTTP Server running on port: 8789, serving: .
Listening on port: 8789
Loading config file '/home/xx/Documents/Dev/gaia/shared/test/unit/test-agent-server.js'

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: listen EADDRINUSE
    at errnoException (net.js:904:11)
    at Server._listen2 (net.js:1042:14)
    at listen (net.js:1064:10)
    at Server.listen (net.js:1138:5)
    at Object.<anonymous> (/home/xx/Documents/Dev/gaia/node_modules/test-agent/lib/node/bin/server.js:71:14)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
make: *** [test-agent-server] Error 8
gaia-test: Stopping the test runner on exit.



What can I do for executing tests ?

Thanks.
Flags: needinfo?(raphael)
(Reporter)

Comment 16

3 years ago
There's nothing cryptic in: "Error: listen EADDRINUSE" :). You already have a process binding this address/port.
You're right, thank you :)

Firefox stats now. How do I open the Test Agent app ? How do I start the tests ?
(Reporter)

Comment 18

3 years ago
(In reply to Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel] from comment #17)
> You're right, thank you :)
> 
> Firefox stats now. How do I open the Test Agent app ? How do I start the
> tests ?

Once you have ./bin/gaiatest running, then you should just save the test file you modified and it will trigger unit testing.
Created attachment 8477976 [details] [diff] [review]
Convert System to new Notification API: BatteryManager.
Attachment #8444166 - Attachment is obsolete: true
Attachment #8477976 - Flags: review?(alive)
Some changements in the patch :
- Create hidePowerSavingNotification function
- Cleanup the code removing the battery specific at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/battery_manager.js#L20
- rough skeleton of unit tests (need help :), cf. error below)


Thanks!



[system-test/unit/battery_manager_test.js] battery goes empty with powersave enabled>

    [system-test/unit/battery_manager_test.js] showPowerSavingNotification and hidePowerSavingNotification should be called correctly
        [system-test/unit/battery_manager_test.js] below threshold with powersave enabled ‣

        Error: Error: fake is not a spy (http://system.gaiamobile.org:8080/common/vendor/sinon/sinon.js?time=1408875818441:4712)
            at onerror (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959:7)

        [system-test/unit/battery_manager_test.js] above threshold with powersave enabled ‣

        Error: Error: fake is not a spy (http://system.gaiamobile.org:8080/common/vendor/sinon/sinon.js?time=1408875818441:4712)
            at onerror (http://system.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959:7)
Comment on attachment 8477976 [details] [diff] [review]
Convert System to new Notification API: BatteryManager.

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

::: apps/system/js/battery_manager.js
@@ +237,5 @@
> +
> +    _powerSaveNotification = powerSavingNotification;
> +  }
> +
> +  function hidePowerSavingNotification() {

Where do you use this function?

::: apps/system/test/unit/battery_manager_test.js
@@ +222,5 @@
> +          });
> +
> +          test('below threshold with powersave enabled', function(done) {
> +            sendLevelChange(0.05);
> +            setTimeout(function() {

We don't want to setTimeout in any unit test. Could you tell me why you need it?
Attachment #8477976 - Flags: review?(alive)
Alex, could you help this contributor since you assign yourself as the mentor of this bug?
(Reporter)

Comment 23

3 years ago
Comment on attachment 8477976 [details] [diff] [review]
Convert System to new Notification API: BatteryManager.

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

Some comments on why your patch fails at test :)

::: apps/system/js/battery_manager.js
@@ +237,5 @@
> +
> +    _powerSaveNotification = powerSavingNotification;
> +  }
> +
> +  function hidePowerSavingNotification() {

Your PowerSaveHandler do not expose showPowerSavingNotification neither hidePowerSavingNotification, so those cannot be used for spying.

::: apps/system/test/unit/battery_manager_test.js
@@ +216,5 @@
> +
> +      suite('showPowerSavingNotification and hidePowerSavingNotification should be called correctly',
> +        function() {
> +          setup(function() {
> +            this.sinon.spy(PowerSaveHandler.showPowerSavingNotification);

This can be moved in the previous setup(), so there is no need for a second subsuite.

@@ +217,5 @@
> +      suite('showPowerSavingNotification and hidePowerSavingNotification should be called correctly',
> +        function() {
> +          setup(function() {
> +            this.sinon.spy(PowerSaveHandler.showPowerSavingNotification);
> +            this.sinon.spy(PowerSaveHandler.hidePowerSavingNotification);

Those sinon.spy calls are wrong, it should be:

this.sinon.spy(PowerSaveHandler, 'showPowerSavingNotification')
this.sinon.spy(PowerSaveHandler, 'hidePowerSavingNotification')

@@ +223,5 @@
> +
> +          test('below threshold with powersave enabled', function(done) {
> +            sendLevelChange(0.05);
> +            setTimeout(function() {
> +              sinon.assert.calledOnce(PowerSaveHandler.showPowerSavingNotification);

I don't see powersave.threshold SettingsListener.observe() getting called back (in the tests), so showPowerSavingNotification is never called.
(Reporter)

Comment 24

3 years ago
Created attachment 8478957 [details] [diff] [review]
battery-manager.diff

Rapahel, I did a couple of changes that may help you fix your tests :)
Flags: needinfo?(raphael)
Created attachment 8485550 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax
Thanks for your help Alexandre.

I've updated my patch. Can't make it works. Did I miss something ?
Flags: needinfo?(raphael)
Attachment #8477976 - Attachment is obsolete: true
(Reporter)

Comment 27

3 years ago
Thanks. Now you need to make it a pull request against the Gaia repo, so that we have a Gaia-Try to look at :). You also need to clear old notifications during the init.
(Reporter)

Updated

3 years ago
Flags: needinfo?(raphael)
Gaia-Try results:
https://tbpl.mozilla.org/?rev=bd3d96ff5ac44b61ee3b4dc6f8676977bad61dfe&tree=Gaia-Try
Flags: needinfo?(raphael)
Created attachment 8485639 [details] [diff] [review]
Convert System to new Notification API: BatteryManager. r=gerard-majax
Attachment #8485550 - Attachment is obsolete: true
(Reporter)

Comment 30

3 years ago
Created attachment 8485658 [details] [review]
Gaia PR
Attachment #8485639 - Attachment is obsolete: true
(Reporter)

Comment 31

3 years ago
(In reply to Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel] from comment #28)
> Gaia-Try results:
> https://tbpl.mozilla.org/
> ?rev=bd3d96ff5ac44b61ee3b4dc6f8676977bad61dfe&tree=Gaia-Try

Thanks, I attached the link of the PR. Looking at the try, I see:
 - Gaia unit tests are failing
 - Linter is failing
https://tbpl.mozilla.org/?rev=a9763f044559f8f077cfaa6d37ff46d531d13dcf&tree=Gaia-Try
- Linter now passes
- Gaia unit tests are still failing. Don't know how to proceed to make them pass... Need help :)
- There is a build error on Gaia JS Integration Test. What can I do ?
(Reporter)

Comment 33

3 years ago
Gijs failure seems unrelated.
(Reporter)

Comment 34

3 years ago
For the unit test error, it's there: https://tbpl.mozilla.org/php/getParsedLog.php?id=47621572&tree=Gaia-Try#error0

> Error: expected showPowerSavingNotification to be called once but was called 0 times

You don't reproduce it locally?

This may be related:
> 12:58:27     INFO -  JavaScript error: app://system.gaiamobile.org/js/battery_manager.js?time=1410206306986, line 146: SettingsListener is null
(Reporter)

Updated

3 years ago
Flags: needinfo?(raphael)
Hey guys, 

:gerard-majax made me aware that this bug exists. We found a need for a Notification wrapper for L10n purposes and we want to reuse NotificationHelper. See bug 1095109. Because it also moves NotificationHelper to use W3C Notification API, I'd like to mark this bug as a dupe of that one.

I apologies for not catching this earlier and for :rlustin's time :(
(Reporter)

Comment 36

3 years ago
Closing per comment 35
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1095109
(Reporter)

Updated

2 years ago
Flags: needinfo?(raphael)
You need to log in before you can comment on or make changes to this bug.