The default bug view has changed. See this FAQ.

Migrate tests to use shared mock_l10n

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: Aniket, Mentored)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

User Story

Replace per-app mock_l10n with /shared/test/unit/mock_l10n.js

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Depends on: 1004973
Whiteboard: [good first bug]
(Reporter)

Updated

3 years ago
Summary: Migrate tests to use shared mock_l10ngr → Migrate tests to use shared mock_l10n
(Reporter)

Updated

3 years ago
Depends on: 906580
(Reporter)

Updated

3 years ago
User Story: (updated)
(Assignee)

Comment 1

3 years ago
please assign this bug to me, i am new here and would like to contribute
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!
Assignee: nobody → aniketisin
(Assignee)

Comment 3

3 years ago
Created attachment 8479429 [details] [review]
PR request
Attachment #8479429 - Flags: review?(gandalf)
(Reporter)

Comment 4

3 years ago
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 :)
Attachment #8479429 - Flags: review?(stas)
Attachment #8479429 - Flags: review?(gandalf)
Attachment #8479429 - Flags: review+
(Reporter)

Comment 5

3 years ago
Also, please look here: https://tbpl.mozilla.org/?rev=28c4705343cb5b30f8e7276b22e8e20ce624f51f&tree=Gaia-Try and clean the Li errors (linting errors).
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?
(Assignee)

Comment 7

3 years ago
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
Blocks: 1035385
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.
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 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!
Attachment #8479429 - Flags: review?(stas)
(Reporter)

Comment 11

3 years ago
Ping?
Flags: needinfo?(aniketisin)
(Assignee)

Comment 12

3 years ago
(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
Flags: needinfo?(aniketisin)
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!
Flags: needinfo?(aniketisin)
(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.
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.
Flags: needinfo?(aniketisin)
Master: https://github.com/mozilla-b2g/gaia/commit/a6c29cc2f048dc08a2f008322699368b7e5cd9b0

Thanks, Aniket, for all your hard work on this!
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
I broke one test when I rebased my branch.  I'll have a follow up in a second.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://github.com/mozilla-b2g/gaia/pull/24931
https://tbpl.mozilla.org/?rev=dd9c33d4cf53&tree=Gaia-Try
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)
Master: https://github.com/mozilla-b2g/gaia/commit/b24be6e23e749b0e896563f15d7d23e037ba8771
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1080556
You need to log in before you can comment on or make changes to this bug.