Closed
Bug 1043643
Opened 10 years ago
Closed 10 years ago
Migrate tests to use shared mock_l10n
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zbraniecki, Assigned: aniketisin, Mentored)
References
Details
(Whiteboard: [good first bug])
User Story
Replace per-app mock_l10n with /shared/test/unit/mock_l10n.js
Attachments
(2 files)
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•10 years ago
|
Summary: Migrate tests to use shared mock_l10ngr → Migrate tests to use shared mock_l10n
Reporter | ||
Updated•10 years ago
|
User Story: (updated)
please assign this bug to me, i am new here and would like to contribute
Comment 2•10 years ago
|
||
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
Attachment #8479429 -
Flags: review?(gandalf)
Reporter | ||
Comment 4•10 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•10 years ago
|
||
Also, please look here: https://tbpl.mozilla.org/?rev=28c4705343cb5b30f8e7276b22e8e20ce624f51f&tree=Gaia-Try and clean the Li errors (linting errors).
Comment 6•10 years ago
|
||
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?
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•10 years ago
|
||
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•10 years ago
|
||
Aniket, feel free to use this patch in your pull request.
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 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)
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/a6c29cc2f048dc08a2f008322699368b7e5cd9b0 Thanks, Aniket, for all your hard work on this!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 17•10 years ago
|
||
I broke one test when I rebased my branch. I'll have a follow up in a second.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/pull/24931 https://tbpl.mozilla.org/?rev=dd9c33d4cf53&tree=Gaia-Try
Comment 19•10 years ago
|
||
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•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/b24be6e23e749b0e896563f15d7d23e037ba8771
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•