Closed Bug 1025629 Opened 7 years ago Closed 7 years ago

Convert System to new Notification API: BatteryManager

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1095109

People

(Reporter: gerard-majax, Assigned: rlustin, Mentored)

References

Details

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

Attachments

(2 files, 6 obsolete files)

apps/system/js/battery_manager.js:
  - sending notification of power saving mode
Whiteboard: [systemsfe][good first bug][mentor=gerard-majax]
Mentor: lissyx+mozillians
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!
Assignee: nobody → raphael
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)
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+
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.
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.
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)
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 ?
(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.
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?
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.
Rapahel, I did a couple of changes that may help you fix your tests :)
Flags: needinfo?(raphael)
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
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.
Flags: needinfo?(raphael)
Attached file Gaia PR
Attachment #8485639 - Attachment is obsolete: true
(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 ?
Gijs failure seems unrelated.
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
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 :(
Closing per comment 35
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1095109
Flags: needinfo?(raphael)
You need to log in before you can comment on or make changes to this bug.