1043643 - Migrate tests to use shared mock_l10n

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[Aniket]

please assign this bug to me, i am new here and would like to contribute

Comment 2

[Staś Małolepszy :stas]

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

[Aniket]

Attachment: PR request

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also, please look here: https://tbpl.mozilla.org/?rev=28c4705343cb5b30f8e7276b22e8e20ce624f51f&tree=Gaia-Try and clean the Li errors (linting errors).

Comment 6

[Staś Małolepszy :stas]

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

[Aniket]

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

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

Attachment: Remove mock_l10n from apps/bookmark and apps/settings

Aniket, feel free to use this patch in your pull request.

Comment 10

[Staś Małolepszy :stas]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ping?

Comment 12

[Aniket]

(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

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

(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

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

Master: https://github.com/mozilla-b2g/gaia/commit/a6c29cc2f048dc08a2f008322699368b7e5cd9b0

Thanks, Aniket, for all your hard work on this!

Comment 17

[Staś Małolepszy :stas]

I broke one test when I rebased my branch.  I'll have a follow up in a second.

Comment 18

[Staś Małolepszy :stas]

https://github.com/mozilla-b2g/gaia/pull/24931
https://tbpl.mozilla.org/?rev=dd9c33d4cf53&tree=Gaia-Try

Comment 19

[Staś Małolepszy :stas]

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)

Comment 20

[Staś Małolepszy :stas]

Master: https://github.com/mozilla-b2g/gaia/commit/b24be6e23e749b0e896563f15d7d23e037ba8771