1078187 - Use the shared mock_l10n in Collections

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Staś Małolepszy :stas]

We're moving away from per-app mozL10n mocks in favor of shared/test/unit/mocks/mock_l10n.js.  This bug is about migrating the Collections app.

Collections suggestions engine relies on mozL10n.get returning an empty string from missing translations.  We'll fix this in the future as part of bug 1020138.  In the meantime, we can stub mozL10n.get.

Comment 1

[Staś Małolepszy :stas]

Attachment: Stub mozL10n.get

PR: https://github.com/mozilla-b2g/gaia/pull/24814
TH: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=863bb310006f

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 8500322 [details] [diff] [review]
Stub mozL10n.get

Julien, can you take a look at this patch and my use of stubs?

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8500322 [details] [diff] [review]
Stub mozL10n.get

Review of attachment 8500322 [details] [diff] [review]:
-----------------------------------------------------------------

Passing the review to a peer. Still here are some comments:

::: apps/collection/test/unit/suggestions_test.js
@@ +47,5 @@
>      });
>    });
>  
>    test('populates options from collections database', function(done) {
> +    this.sinon.stub(navigator.mozL10n, 'get').returns('');

wondering if it makes sense to put it in the setup?

@@ +75,5 @@
> +          return 'fromL10nFile';
> +        default:
> +          return '';
> +      }
> +    });

Could be more readable to do it like this:

  this.sinon.stub(navigator.mozL10n, 'get').returns(''); // not needed if it's done in setup
  navigator.mozL10n.get.withArgs('collection-categoryId-1').returns('fromL10nFile');
  navigator.mozL10n.get.withArgs('collection-categoryId-2').returns('fromL10nFile');

Although that's arguable :) Maybe in this case we can use a variable to keep the stub.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8500322 [details] [diff] [review]
Stub mozL10n.get

Review of attachment 8500322 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me, thanks.

::: apps/collection/js/suggestions.js
@@ -4,5 @@
>  
>  (function(exports) {
>  
>    const l10nKey = 'collection-categoryId-';
> -  var _ = navigator.mozL10n.get;

Why don't you like this? I would personally prefer to keep it.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Kevin Grandon :kgrandon from comment #4)

> Why don't you like this? I would personally prefer to keep it.

It needs to be defined at script-parsing time, which is not easy when using the mock (unless we load the file in suiteSetup, but it's not really nice)..

Comment 6

[Staś Małolepszy :stas]

Thanks for the reviews!

(In reply to Kevin Grandon :kgrandon from comment #4)

> >    const l10nKey = 'collection-categoryId-';
> > -  var _ = navigator.mozL10n.get;
> 
> Why don't you like this? I would personally prefer to keep it.

If I keep the _, it'll hold the reference to the original method, not the stub.

(In reply to Julien Wajsberg [:julienw] from comment #3)
> > +    this.sinon.stub(navigator.mozL10n, 'get').returns('');
> 
> wondering if it makes sense to put it in the setup?

Returning an empty string is an edge-case scenario and I think I prefer to define it in the test itself in order to better control and emphasize it.

> Although that's arguable :) Maybe in this case we can use a variable to keep
> the stub.

I quite enjoy using this.sinon and not having to manually restore stubbed methods.  I'll leave this code as-is.

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Staś Małolepszy :stas from comment #6)

> > Although that's arguable :) Maybe in this case we can use a variable to keep
> > the stub.
> 
> I quite enjoy using this.sinon and not having to manually restore stubbed
> methods.  I'll leave this code as-is.

Yeah, me too, I meant something like this:

  var getStub = this.sinon.stub(navigator.mozL10n, 'get').returns('');
  getStub.withArgs(...).returns(...)
  getStub.withArgs(...).returns(...)

You still don't need to restore yourself, but it's maybe more readable than what I originally proposed.

Comment 8

[Staś Małolepszy :stas]

Ah, I see. Is that better style than the switch that I went for?  I'll be happy to change it if you feel like it.

Comment 9

[Julien Wajsberg [:julienw]]

I think it's better style and make the code more readable. But up to you :)

Comment 10

[Staś Małolepszy :stas]

I totally forgot about this bug ^^.  I applied Julien's last comments and landed in

https://github.com/mozilla-b2g/gaia/commit/c0344d4504b27f4666cd4b538612204b9ef52fa6

There was an unrelated failure in Gu (calendar-test/unit/day_observer_test.js] day_observer) in https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=2d654e906618 which I also confirmed was happening on vanilla master.