Last Comment Bug 1043643 - Migrate tests to use shared mock_l10n
: Migrate tests to use shared mock_l10n
Status: RESOLVED FIXED
[good first bug]
:
Product: Firefox OS
Classification: Client Software
Component: Gaia (show other bugs)
: unspecified
: x86 All
-- normal (vote)
: ---
Assigned To: Aniket
:
:
Mentors: Zibi Braniecki [:gandalf][:zibi]
Depends on: 906580 1004973 1080556
Blocks: 1035385
  Show dependency treegraph
 
Reported: 2014-07-24 15:15 PDT by Zibi Braniecki [:gandalf][:zibi]
Modified: 2014-10-20 00:55 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
PR request (46 bytes, text/x-github-pull-request)
2014-08-26 15:54 PDT, Aniket
gandalf: review+
Details | Review | Splinter Review
Remove mock_l10n from apps/bookmark and apps/settings (15.46 KB, patch)
2014-09-03 04:11 PDT, Staś Małolepszy :stas
no flags Details | Diff | Splinter Review

User Story
Replace per-app mock_l10n with /shared/test/unit/mock_l10n.js      
Description User image Zibi Braniecki [:gandalf][:zibi] 2014-07-24 15:15:35 PDT
Apps like bluetooth, collection, email, ftu, search, settings, verticalhome, video, wappush use their own mock_l10n. We should migrate all of those to shared/test/unit/mock_l10n.js

There are also tests the create their own MockL10n within the test file[1]. We should clean that too.

[1] https://github.com/mozilla-b2g/gaia/blob/62eedafb0657bbec8941f5bdaa919b5a59f25db1/apps/settings/test/unit/findmydevice_panel_test.js#L35-L41

It's a fairly easy first bug. I'm happy to mentor
Comment 1 User image Aniket 2014-08-25 08:27:20 PDT
please assign this bug to me, i am new here and would like to contribute
Comment 2 User image Staś Małolepszy :stas 2014-08-26 07:10:39 PDT
Hey Aniket, thanks for stepping in!  Don't hesitate to ping me or gandalf on IRC if you have questions, we'll be more than happy to help!
Comment 3 User image Aniket 2014-08-26 15:54:22 PDT
Created attachment 8479429 [details] [review]
PR request
Comment 4 User image Zibi Braniecki [:gandalf][:zibi] 2014-08-26 16:17:15 PDT
Comment on attachment 8479429 [details] [review]
PR request

That looks good! Please, remove the test update from video_test.js.

The reason why it doesn't work is because the test is written in a way that depends on mozL10n.once not being fired in the test (while shared mock_l10n does fire it).

We should file a separate bug to update the whole test file to use shared mock_l10n.

I left some comment notes in the patch, but overall it looks great!

I'm settings Stas as the second reviewer, since he knows about tests more than I do :)
Comment 5 User image Zibi Braniecki [:gandalf][:zibi] 2014-08-26 16:18:14 PDT
Also, please look here: https://tbpl.mozilla.org/?rev=28c4705343cb5b30f8e7276b22e8e20ce624f51f&tree=Gaia-Try and clean the Li errors (linting errors).
Comment 6 User image Staś Małolepszy :stas 2014-08-28 05:14:06 PDT
Comment on attachment 8479429 [details] [review]
PR request

Hey Aniket -- the patch looks good but before I give it a final r+, please remove npm-debug.log from it.

I also noticed that the following mock_l10n.js files are still present on your branch:

apps/video/test/unit/mock_l10n.js
apps/emergency-call/test/unit/mock_l10n.js
apps/collection/test/unit/mock_l10n.js
apps/bookmark/test/unit/mock_l10n.js
apps/communications/contacts/test/unit/mock_l10n.js
apps/homescreen/test/unit/mock_l10n.js
apps/settings/test/unit/mock_l10n.js

Are they required?  Can we remove them?
Comment 7 User image Aniket 2014-08-28 08:01:46 PDT
The one in emergency-call has been removed, the rest of the apps have some tests which do not sync well with the commoon mock_l10n, and hence they use their own. I have created a PR
Comment 8 User image Staś Małolepszy :stas 2014-09-03 04:09:53 PDT
Hi Aniket!

Please rebase your patch onto the current master.  2.1 branched off yesterday, and we should be using the latest master code as the base.


(In reply to Staś Małolepszy :stas from comment #6)
> apps/video/test/unit/mock_l10n.js
> apps/collection/test/unit/mock_l10n.js
> apps/communications/contacts/test/unit/mock_l10n.js
> apps/homescreen/test/unit/mock_l10n.js

Can you file four bugs, one for each of these mocks in their respective components, blocking bug 1035385?

> apps/bookmark/test/unit/mock_l10n.js
> apps/settings/test/unit/mock_l10n.js

I was able to remove these two files, I'll attach a patch for you to apply.
Comment 9 User image Staś Małolepszy :stas 2014-09-03 04:11:17 PDT
Created attachment 8483403 [details] [diff] [review]
Remove mock_l10n from apps/bookmark and apps/settings

Aniket, feel free to use this patch in your pull request.
Comment 10 User image Staś Małolepszy :stas 2014-09-03 04:21:37 PDT
Comment on attachment 8479429 [details] [review]
PR request

Canceling the review for now.  Please re-request when you address my comment 8 and comment 9.  Thanks!
Comment 11 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-08 10:00:39 PDT
Ping?
Comment 12 User image Aniket 2014-09-08 10:25:42 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #11)
> Ping?

Sorry for the late reply internet in my institute was down due to some problem, it is under repair hence communication problem
Comment 13 User image Staś Małolepszy :stas 2014-10-01 06:08:43 PDT
Hi Aniket -- are you still working on this?  I feel this is very close to being landable, just needs a final touch from comment 9.  Would you like to finish this?  Do you need my help?  Just let me know!
Comment 14 User image Staś Małolepszy :stas 2014-10-06 03:42:16 PDT
(In reply to Staś Małolepszy :stas from comment #8)
> > apps/video/test/unit/mock_l10n.js
> > apps/collection/test/unit/mock_l10n.js
> > apps/communications/contacts/test/unit/mock_l10n.js
> > apps/homescreen/test/unit/mock_l10n.js
> 
> Can you file four bugs, one for each of these mocks in their respective
> components, blocking bug 1035385?

I filed bug 1078183, bug 1078187, bug 1078212 and bug 1078214.
Comment 15 User image Staś Małolepszy :stas 2014-10-06 05:41:08 PDT
I took Aniket's pull request, applied my patch and rebased it on top of the current master:

https://github.com/mozilla-b2g/gaia/pull/24810
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=80241643adea

I think this is good for landing now.
Comment 16 User image Staś Małolepszy :stas 2014-10-08 00:41:34 PDT
Master: https://github.com/mozilla-b2g/gaia/commit/a6c29cc2f048dc08a2f008322699368b7e5cd9b0

Thanks, Aniket, for all your hard work on this!
Comment 17 User image Staś Małolepszy :stas 2014-10-08 02:36:59 PDT
I broke one test when I rebased my branch.  I'll have a follow up in a second.
Comment 19 User image Staś Małolepszy :stas 2014-10-08 04:05:55 PDT
If anyone's looking for this, here's the broken Gu test:

INFO - TEST-UNEXPECTED-FAIL | settings/test/unit/panels/root/bluetooth_item_test.js | BluetoothItem "before all" hook - Error: Load failed: unit/mock_l10n: app://settings.gaiamobile.org/test/unit/mock_l10n.js?bust=1412759831948 (app://settings.gaiamobile.org/js/vendor/alameda.js?time=1412759831850:893)

Note You need to log in before you can comment on or make changes to this bug.