apps/system/js/battery_manager.js: - sending notification of power saving mode

Hi! I'm interested to fix this bug. Can somebody assigns it to me? Thanks!

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 :)

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?

Alive, since you wrote quite a lot of this, you can probably give better review inputs.

The changes looks sane, but please make sure battery_manager_test passes.

Hi, please make a github pull request and have the CI run your patch.

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

battery_manager_test passes, but I don't now if power saving mode notification is tested.

(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?

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.

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.

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?

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 :)

Thanks for your help Alexandre. I've updated my patch. Can't make it works. Did I miss something ?

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.

Gaia-Try results: https://tbpl.mozilla.org/?rev=bd3d96ff5ac44b61ee3b4dc6f8676977bad61dfe&tree=Gaia-Try

(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

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