Closed Bug 1004973 Opened 10 years ago Closed 10 years ago

Unify mock_l10n

Categories

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

Product:
Firefox OS Graveyard
Component:
Gaia::L10n
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: stas)

References

(
URL
)

Details

Attachments

(1 file, 5 obsolete files)

Fix review nits and TBPL failures
17.29 KB, patch
julienw
: review+
Details | Diff | Splinter Review 
Currently we have mock_l10n in:

 - bluetooth
 - bookmark
 - communication/contacts
 - communication/ftu
 - email
 - homescreen
 - search
 - settings
 - sms
 - system
 - video
 - wappush

We should merge them all into a single mock_l10n that will live in shared/test/unit/mocks.
Assignee: nobody → gandalf
URL: https://github.com/zbraniecki/gaia/tr...
Blocks: 1006359
Priority: -- → P4
Attached patch patch (obsolete) — — Splinter Review 
This patch adds MockL10n and MockLazyL10n.

My goal was to stay close to how real MozL10n work, but at the same time stay simple so that anyone can read the MockL10n and understand how it works.

I was able to use it for five apps, so I believe it's good enough to push to master and start transitioning apps to it.
Attachment #8426822 - Flags: feedback?(stas)
Looks too complicated for my taste... I like the simplicity of the mock we have in the SMS app...
Julien -- I agree with you but we also de-prioritized the review of this in favor of other 2.0-targeting bugs.  I'll come back to this bug next week.
I tend to also agree with Julien, but I find it a tough balance to strike between realistically representing how l10n.js works (very complex) and making it easy to use for tests.

Let's get back to this conversation after 2.0. I'd love to land it soon after.
In tests I write, I don't really care about the fact that l10n.js does what it should, but only that the right calls were made. Possibly updating the data-l10n-id when "localize" is called to make it more "realistic".

About once/ready, this can be done in tests very easily using sinon stubs' "yield" function, testing being called several times, etc. We took a path of not using sinon in mocks directly but rather use it in tests only.
Depends on: 1022558
Comment on attachment 8426822 [details] [diff] [review]
patch

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

My general feeling is that there is way too much implementation in this mock.  Instead of faking the mozL10n's behavior, it actually implements some of them, which is bad practice for unit tests.  Unit tests should just test what's being tested and should not be prone to implementation errors of dependencies. Otherwise, we'd just use the real mozL10n.

I know your goal was something that we'd discussed before, i.e. being able to change MockL10n._config.resourceLoadingTime and see which tests break.  I now think that this particular goal would be perhaps better served by integration tests which use actual DOM and actual l10n resources.  In unit tests, the DOM is often a fake one, there are no startup events like DOMContentLoaded and the JS components are initialized manually for the purposes of the particular suite of tests.

When I say 'too much implementation', I mean having a Context object in the mock, with a state, returning actual strings that are run through a placeable-resolving logic.  That's just too many potential points of failure.  All of this should be tested in mozL10n's unit tests already, and we don't need to risk running into mozL10n errors in other apps' tests.

Instead, I believe that we should just have an object that exposes the same API as the real mozL10n, but doesn't have any (or has very very little) logic inside.

Here's how I think most of the interaction with MockL10n should look like:

  suiteSetup(function(done) {

    // Step 1.  Prepare the DOM.  Use the real one:
    loadBodyHTML('/index.html');
    // ...or a fake one:
    stubById = this.sinon.stub(document, 'getElementById');
    stubById.returns(document.createElement('div'));

    // Step 2.  Use MockL10n.
    // realMozL10n is actually 'undefined', so maybe it's not even needed.
    realMozL10n = navigator.mozL10n;
    navigator.mozL10n = MockL10n;

    // Step 3.  Initialize the tested component manually.
    // Either call it's init method if the component has been required before,
    // but that likely means that in the source code we need to do:
    //   if(navigator.mozL10n) navigator.mozL10n.once(init);
    // which is bad.  See also bug 1022558.
    MyComponent.init()

    // A better approach is to require the tested component only now.
    // mozL10n is the mocked one at this point.
    requireApp('app/js/my_component.js', done);
  });

We could also get rid of Step 2 by using the MocksHelper interface and having the following methods defined on MockL10n:
  - mSetup,
  - mTeardown,
  - mSuiteSetup,
  - mSuiteTeardown

See https://github.com/mozilla-b2g/gaia/blob/8b7ecdfbd5bca2cad1e46dd2f9b0bafa74adf931/shared/test/unit/mocks/mocks_helper.js for how these methods are called, https://github.com/mozilla-b2g/gaia/blob/8d0d5494010ad3a13709b4f0858f7bf2e6c61889/shared/test/unit/mocks/mock_icc_helper.js for an example implementation and the following two URLs for an example of use in actual tests:

https://github.com/mozilla-b2g/gaia/blob/8b7ecdfbd5bca2cad1e46dd2f9b0bafa74adf931/apps/system/test/unit/bootstrap_test.js#L49-L58
https://github.com/mozilla-b2g/gaia/blob/8b7ecdfbd5bca2cad1e46dd2f9b0bafa74adf931/apps/system/test/unit/bootstrap_test.js#L71

The overarching goal of a MockL10n designed in the above way is to not get in the way of testing other things.  Developers want their code to be able to call mozL10n methods safely, without raising TypeErrors on unknown methods.

If, for any reason, a developer does care about what this fake mozL10n does, than they should explicitly say so by using stub, as Julien suggested.  Here's an example from the System app:

  test('Dialog title', function() {
    var spyTranslate = this.sinon.spy(MockL10n, 'translate');
    nfcDialog.show();
    assert.isTrue(spyTranslate.calledOnce);
  });

I'll prepare a simple alternative proposal and attach it here to get your thoughts.

::: shared/test/unit/mocks/mock_l10n.js
@@ +39,5 @@
> +      var value = str.replace(rePlaceables, function(match, id) {
> +        if (ctxdata && ctxdata.hasOwnProperty(id) &&
> +          (typeof ctxdata[id] === 'string' ||
> +           (typeof ctxdata[id] === 'number' && !isNaN(ctxdata[id])))) {
> +             return ctxdata[id];

Too much implementation.

@@ +64,5 @@
> +        if (pos !== -1) {
> +          ctx.cbs.splice(pos, 1);
> +        }
> +        callback();
> +      }).bind(this);

Too much implementation.

@@ +87,5 @@
> +
> +
> +    /* For unit testing: */
> +    _setLocaleResources: function(lang, res) {
> +      ctx.locales[lang] = res;

Why would we want to use real resources for unit testing?
Attachment #8426822 - Flags: feedback?(stas) → feedback-
(In reply to Staś Małolepszy :stas from comment #6)

> @@ +64,5 @@
> > +        if (pos !== -1) {
> > +          ctx.cbs.splice(pos, 1);
> > +        }
> > +        callback();
> > +      }).bind(this);
> 
> Too much implementation.

Sorry, I accidentally copied the same comment twice. Here I meant to say that 

  var pos = ctx.cbs.indexOf(callback);

should read:

  var pos = ctx.cbs.indexOf(callAndRemove);
Attached patch Alternative proposal (WIP) (obsolete) — — Splinter Review 
Here's what I've been thinking about.  Note that this is WIP and it's not complete.  It needs the DateTimeFormat part and an equivalent for LazyL10n.

Here's how to use it:

setup.js:
---------

require('/shared/test/unit/mocks/mocks_helper.js');

my_component_test.js:
---------------------

requireApp('system/shared/test/unit/mocks/mock_l10n.js');

var mocksForMyComponent = new MocksHelper([
  'L10n'
]).init();

suite('app/myComponent', function() {
  var stubById;

  mocksForBootstrap.attachTestHelpers();

  suiteSetup(function(done) {
    stubById = this.sinon.stub(document, 'getElementById');
    stubById.returns(document.createElement('div'));
    
    requireApp('app/js/my_component.js', function() {
      MockL10n.mTriggerEventListeners('ready');
      done();
    });
  });

});

* * *

It might even be possible to not require the MockL10n.mTriggerEventListeners('ready') call and instead implement mozL10n.once as setTimout(handler).  I also need to verify if mTriggerEventListeners should call the handlers synchronously or asynchronously. I'll report back later today.
Attachment #8437719 - Flags: feedback?(gandalf)
Attached patch Even simpler (obsolete) — — Splinter Review 
After a few more discussions with myself, I decided that I'd like to start with the minimal, lowest-denominator-style, MockL10n, so that we can start porting existing unit tests to it and, perhaps more importantly, encourage developers to start using it in new tests.

I removed any form of event handler registration and instead implemented both mozL10n.ready and mozL10n.once with a simple setTimeout.

In a follow-up bug, I'd like to add a mock-only method which re-fires .ready() callbacks, so that developers can start writing tests which simulate a language change.

Sadly, I also realized that we can't currently take advantage of MocksHelper, because it currently only mocks things directly on window.  There might be a way to make it work, but I don't know it.

I added DateTimeFormat and MockLazyL10n inthis patch.  I can take this bug if you want.  This is still a demonstration of my thinking.  Before I work on a reviewable patch, I'd like to 1) get your go-ahead for this approach and 2) port one app to using this new shared MockL10n.
Attachment #8437719 - Attachment is obsolete: true
Attachment #8437719 - Flags: feedback?(gandalf)
Attachment #8437792 - Flags: feedback?(gandalf)
(In reply to Staś Małolepszy :stas from comment #9)
> Sadly, I also realized that we can't currently take advantage of
> MocksHelper, because it currently only mocks things directly on window. 
> There might be a way to make it work, but I don't know it.
I think we should extend MocksHelper to allow attaching mocks to the navigator object. We have this use case in every unit test and it's annoying. In another bug though.
(In reply to Anthony Ricaud (:rik) from comment #10)
> (In reply to Staś Małolepszy :stas from comment #9)
> > Sadly, I also realized that we can't currently take advantage of
> > MocksHelper, because it currently only mocks things directly on window. 
> > There might be a way to make it work, but I don't know it.
> I think we should extend MocksHelper to allow attaching mocks to the
> navigator object. We have this use case in every unit test and it's
> annoying. In another bug though.

We're planning to move mozL10n to document (bug 1020250), so we'll need to cover that too.
Comment on attachment 8437792 [details] [diff] [review]
Even simpler

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

I understand that you want a simpler module than what I suggested, and I think you're right and we should go for a simpler approach. But I'm worried about oversimplifying things to the point where they don't test anything anymore.

The trick with l10n library is that it's extremely easy to visually test the "common" scenario and developers do this all the time. They write code, launch it and check if the en-US string appears. It's significantly harder for them to test all other scenarios - what if resources load late, what if language changes, what if one of the edge case arguments is not passed as a string.

I believe that mockl10n should help developers catch edge cases by:

1) being very strict with what input API accepts
2) being fairly realistic about various bootstrapping scenarios
3) being fairly realistic about resource handling

Right now I don't see too much being tested here at all. Out of all the bugs with how l10n api is used that I saw in the last three months, I'm not sure if your mockl10n would catch anything.

On the other hand, I have little experience with writing mock libraries, so I may be wrong with my assumptions on what mockl10n should really test.

What do you think?

::: shared/test/unit/mocks/mock_l10n.js
@@ +7,5 @@
> +      if (args) {
> +        return key + ' ' + JSON.stringify(args);
> +      }
> +      return key;
> +    },

I'm not sure if I like it. There are multiple things that I saw going wrong with this approach:

1) Developers start testing against the value returned, see https://github.com/mozilla-b2g/gaia/blob/7871b14da079257e8746fe963197cb36938b2c5f/apps/system/test/unit/lockscreen_conn_info_manager_test.js#L270-L272 - I saw many examples of such code.

2) There's no way to test if the value is "in the right language"

3) It doesn't test if the context is ready. That means that your tests will not catch code trying to mozL10n.get before the context is ready

4) You don't test types, so code trying to use object as a key or object as a value in args (see bug 994417 and bug 1005862) will not fail.
Comment on attachment 8437792 [details] [diff] [review]
Even simpler

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

::: shared/test/unit/mocks/mock_l10n.js
@@ +7,5 @@
> +      if (args) {
> +        return key + ' ' + JSON.stringify(args);
> +      }
> +      return key;
> +    },

> 1) Developers start testing against the value returned, 

But that's the point, right?

> 2) There's no way to test if the value is "in the right language"

But that's the role of the l10n library, why users of the lib should test this?

> 3) It doesn't test if the context is ready. That means that your tests will not catch code trying to mozL10n.get before the context is ready

This could be done in tests by stubbing "get" and make it throwing. But I agree with you here, it would be too easy to forget this case if it's important. What do you suggest?

> 4) You don't test types, so code trying to use object as a key or object as a value in args (see bug 994417 and bug 1005862) will not fail.

Should be easy to throw here if the arguments do not have the right type.
(In reply to Julien Wajsberg [:julienw] from comment #13)
> > 1) Developers start testing against the value returned, 
> 
> But that's the point, right?

Not sure. That feels to me like a risky dirty hack. If you want to test if you code asked for entity "foo" with args "{bar: 'baz'}", you should not write:

assert.equals(node.textContent, "foo{bar:'baz'}");

imho. Instead, you should use sinon, right? So maybe reduce temptation and don't return anything?

> > 2) There's no way to test if the value is "in the right language"
> 
> But that's the role of the l10n library, why users of the lib should test
> this?

Well, except when you manipulate languages in ways we don't control (email's tmpl which stores localized DOM).
(In reply to Zibi Braniecki [:gandalf] from comment #14)
> (In reply to Julien Wajsberg [:julienw] from comment #13)
> > > 1) Developers start testing against the value returned, 
> > 
> > But that's the point, right?
> 
> Not sure. That feels to me like a risky dirty hack. If you want to test if
> you code asked for entity "foo" with args "{bar: 'baz'}", you should not
> write:
> 
> assert.equals(node.textContent, "foo{bar:'baz'}");
> 
> imho. Instead, you should use sinon, right? So maybe reduce temptation and
> don't return anything?

Not really. You don't want to test that you asked for the right entity. You want to test that what you asked is also at the right place.

I admit it was an easy hack from when we didn't use sinon. But it works fine and is IMO easier to use than anything else we can do.

That said, in a time where we use localize, we mostly either check for data-l10n-id or that localize was called with the right arguments.

> 
> > > 2) There's no way to test if the value is "in the right language"
> > 
> > But that's the role of the l10n library, why users of the lib should test
> > this?
> 
> Well, except when you manipulate languages in ways we don't control (email's
> tmpl which stores localized DOM).

Would anything you put in the Mock help this case?
Gandalf: A lot of your arguments against a simple mock could be solved by having the l10n lib be more strict.

> I believe that mockl10n should help developers catch edge cases by:
> 1) being very strict with what input API accepts
The lib should be strict about what it accepts. Throw errors, they will either break the functionality or appear in the logcat.

> 3) It doesn't test if the context is ready. That means that your tests will not catch code trying to mozL10n.get before the context is ready
The lib show throw if the context is not ready.

> 4) You don't test types, so code trying to use object as a key or object as a value in args (see bug 994417 and bug 1005862) will not fail.
The lib should throw if the value is not the right type. I know I've argued in the opposite direction in those bugs but that's because the lib was strict without throwing.

Unit tests in general are just here to check that the object under test is behaving as expected. If a bug is discovered because the l10n lib was misused by an object, then the patch to fix the bug will add a unit test to verify that it is now calling the l10n mock properly.
Also, having a complex mock means that every behaviour change to the l10n lib will have to be reflected in the mock. That is usually a waste of human resources.
(In reply to Julien Wajsberg [:julienw] from comment #13)
> > 3) It doesn't test if the context is ready. That means that your tests will not catch code trying to mozL10n.get before the context is ready
> 
> This could be done in tests by stubbing "get" and make it throwing. But I
> agree with you here, it would be too easy to forget this case if it's
> important. What do you suggest?

idea:

make "once" and "ready" start a resolved Promise (so that the code executes in an asynchronous way without risking timeout); in the then handler: you flip a boolean to true. Then in other methods, if the boolean is false, you throw, otherwise you do what's expected.

In the mTeardown you flip the boolean back to false.
Comment on attachment 8437792 [details] [diff] [review]
Even simpler

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

thanks for explanations. I think I understand the goal of mock_l10n a bit better now.
Attachment #8437792 - Flags: feedback?(gandalf) → feedback+
(In reply to Anthony Ricaud (:rik) from comment #16)
> > 3) It doesn't test if the context is ready. That means that your tests will not catch code trying to mozL10n.get before the context is ready
> The lib show throw if the context is not ready.

Problem is that it's racy; in real world, sometimes the context will be ready and sometimes it might not. It depends on how fast some resources are loaded.
One more interesting twist to consider. With landing Mutation Observer in bug 992473 we will have *real* mozL10n running next to mockL10n in tests.

All I had to do to fix two bugs today was to ensure that onMutations handler is bound to the real one instead of just using navigator.mozL10n which in tests became MockL10n.

Not sure how it should affect the API of MockL10n.
I don't see why the move to mutation observers means the real lib and the mock will both be loaded in a unit test. Unit tests shouldn't insert the real lib.
(In reply to Anthony Ricaud (:rik) from comment #22)
> I don't see why the move to mutation observers means the real lib and the
> mock will both be loaded in a unit test. Unit tests shouldn't insert the
> real lib.

This is the build: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=071008d56069

Those are the tests:
 - https://github.com/mozilla-b2g/gaia/blob/master/apps/video/test/unit/video_test.js#L36
 - https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/test/unit/sms_test.js#L117

In those cases we load the real index.html which has real l10n.js that have real MO operating.
(In reply to Zibi Braniecki [:gandalf] from comment #23)

> https://github.com/mozilla-b2g/gaia/blob/master/apps/video/test/unit/
> video_test.js#L36
>  -
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/test/unit/sms_test.
> js#L117
> 
> In those cases we load the real index.html which has real l10n.js that have
> real MO operating.

Those test files explicitly load shared/js/l10n.js:

https://github.com/mozilla-b2g/gaia/blob/master/apps/video/test/unit/video_test.js#L7
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/test/unit/sms_test.js#L117
And I'm guessing that with loadBodyHTML, the HTML has the locales.ini link, so the real mozL10n actually initializes correctly and registers its own mutation observer.
(In reply to Staś Małolepszy :stas from comment #26)
> And I'm guessing that with loadBodyHTML, the HTML has the locales.ini link,
> so the real mozL10n actually initializes correctly and registers its own
> mutation observer.

I was guessing wrong. loadBodyHTML has nothing to do with it.  mozL10n initializes also when no ini links are found:

https://github.com/mozilla-b2g/gaia/blob/7b68726ed274a59aa31202eb406abb246b527510/shared/js/l10n.js#L1297
Assignee: gandalf → stas
Priority: P4 → P2
sms_test loads l10n.js but not the mock :)
(In reply to Julien Wajsberg [:julienw] from comment #15)
> (In reply to Zibi Braniecki [:gandalf] from comment #14)
> > Not sure. That feels to me like a risky dirty hack. If you want to test if
> > you code asked for entity "foo" with args "{bar: 'baz'}", you should not
> > write:
> > 
> > assert.equals(node.textContent, "foo{bar:'baz'}");
> > 
> > imho. Instead, you should use sinon, right? So maybe reduce temptation and
> > don't return anything?
> 
> Not really. You don't want to test that you asked for the right entity. You
> want to test that what you asked is also at the right place.
> 
> I admit it was an easy hack from when we didn't use sinon. But it works fine
> and is IMO easier to use than anything else we can do.

I think Gandalf's point was about being explicit about what you're testing, which is better achieved with sinon.

Instead of having the mock return the id+args, I believe it's better to do something like this:

  setup(function() {
    this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);
  });

  test('file too large', function() {
    sinon.assert.calledOnce(navigator.mozL10n.get);
    sinon.assert.calledWith(navigator.mozL10n.get, 'files-too-large', { n: 1 });
    sinon.assert.calledWith(window.alert, 'files-too-large');
  });

This allows us to make sure that mozL10n.get is called exactly once, with the right arguments and since we're already spying on it, we might just as well stub it for good measure and make it return what we want. This puts the test author in control and has them write tests consciously, especially when the mozL10n.get calls are nested.  Think DateTimeFormat.localeFormat and friends.
Attached patch Shared MockL10n and SMS tests (obsolete) — — Splinter Review 
Here's the new MockL10n which can be found in shared/test/unit/mocks/mock_l10n.js.

As an example of usage, I ported all SMS app's tests to it.  Gandalf, Julien, can you review this for me please?  Rik, since you've helped us narrow down the behavior of the Mock, would you mind taking a look as well?  Thanks!

Pull request: https://github.com/mozilla-b2g/gaia/pull/20395

Yesterday's builds:

https://travis-ci.org/mozilla-b2g/gaia/builds/27365469
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=d8f9048c2eb3

Today's builds (I cleaned up a few more SMS tests):

https://travis-ci.org/mozilla-b2g/gaia/builds/27389340
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=d2e8f419fdf1
Attachment #8426822 - Attachment is obsolete: true
Attachment #8437792 - Attachment is obsolete: true
Attachment #8439090 - Flags: review?(gandalf)
Attachment #8439090 - Flags: review?(felash)
Attachment #8439090 - Flags: feedback?(anthony)
Comment on attachment 8439090 [details] [diff] [review]
Shared MockL10n and SMS tests

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

::: shared/test/unit/mocks/mock_l10n.js
@@ +37,5 @@
> +    relativeParts: identity
> +  };
> +
> +  function identity(arg) {
> +    return arg;

I was tempted to implement this as:

  return '' + Math.random();

in order to force the use of stubs in tests.  It would be bit radical, although I'm sure it'd help in the long run.
Comment on attachment 8439090 [details] [diff] [review]
Shared MockL10n and SMS tests

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

I like it.
Attachment #8439090 - Flags: review?(gandalf) → review+
Status: NEW → ASSIGNED
(In reply to Staś Małolepszy :stas from comment #30)
> (In reply to Julien Wajsberg [:julienw] from comment #15)
> > (In reply to Zibi Braniecki [:gandalf] from comment #14)
> > > Not sure. That feels to me like a risky dirty hack. If you want to test if
> > > you code asked for entity "foo" with args "{bar: 'baz'}", you should not
> > > write:
> > > 
> > > assert.equals(node.textContent, "foo{bar:'baz'}");
> > > 
> > > imho. Instead, you should use sinon, right? So maybe reduce temptation and
> > > don't return anything?
> > 
> > Not really. You don't want to test that you asked for the right entity. You
> > want to test that what you asked is also at the right place.
> > 
> > I admit it was an easy hack from when we didn't use sinon. But it works fine
> > and is IMO easier to use than anything else we can do.
> 
> I think Gandalf's point was about being explicit about what you're testing,
> which is better achieved with sinon.
> 
> Instead of having the mock return the id+args, I believe it's better to do
> something like this:
> 
>   setup(function() {
>     this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);
>   });
> 
>   test('file too large', function() {
>     sinon.assert.calledOnce(navigator.mozL10n.get);
>     sinon.assert.calledWith(navigator.mozL10n.get, 'files-too-large', { n: 1
> });
>     sinon.assert.calledWith(window.alert, 'files-too-large');
>   });
> 
> This allows us to make sure that mozL10n.get is called exactly once

Is it really useful to know this?

> , with
> the right arguments and since we're already spying on it, we might just as
> well stub it for good measure and make it return what we want. This puts the
> test author in control and has them write tests consciously, especially when
> the mozL10n.get calls are nested.  Think DateTimeFormat.localeFormat and
> friends.

What we want to test is the external behavior. I don't care that mozL10n.get was called. What I care is that alert is called with a localized value, which happens to be return by mozL10n.get.

When the mock returns a concatenation of id + arg, we can do this in just one line here, whereas your changes make it necessary to have 3 lines.

Still, I agree that having a test that depends on an external file behavior is weird and bad practice. Therefore I'd suggest a middle ground: in the SMS tests that depends on this behavior, stub "get" to do what the previous mock was doing.
Comment on attachment 8439090 [details] [diff] [review]
Shared MockL10n and SMS tests

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

::: apps/sms/test/unit/attachment_test.js
@@ +500,4 @@
>        setup(function() {
>          this.sinon.spy(window, 'MozActivity');
>          this.sinon.stub(window, 'alert');
> +        this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);

seems useless with the current mock?

@@ +512,4 @@
>        });
>  
>        test('No handler for this image', function() {
> +        navigator.mozL10n.get.returnsArg(0);

this too?

::: apps/sms/test/unit/settings_test.js
@@ +242,5 @@
> +      test('getSimNameByIccId returns the correct name for SIM 2', function() {
> +        assert.equal(Settings.getSimNameByIccId('SIM 2'), 'sim-name');
> +        sinon.assert.calledOnce(navigator.mozL10n.get);
> +        sinon.assert.calledWith(navigator.mozL10n.get, 'sim-name', { id: 2 });
> +      });

see what I mean? It's just noise to me...

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +575,4 @@
>          return this.selectedInputs;
>        }.bind(this));
>        this.sinon.stub(MessageManager, 'getMessages');
> +      this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);

looks useless?

::: apps/sms/test/unit/thread_ui_test.js
@@ +2765,4 @@
>          return {};
>        });
>        localize = this.sinon.spy(navigator.mozL10n, 'localize');
> +      this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);

useless

@@ +2778,3 @@
>          element = document.getElementById('message-' + message.id);
>          notDownloadedMessage = element.querySelector('.not-downloaded-message');
>          button = element.querySelector('button');

nit: you added a useless empty line

@@ -5801,5 @@
>            ThreadUI.draft = null;
>            Compose.append('some stuff');
>            ThreadUI.recipients.add({number: '456789'});
>  
> -          ThreadUI.beforeEnter(transitionArgs);

why moving this?

@@ +5823,5 @@
>          test('updates the header', function() {
> +          ThreadUI.beforeEnter(transitionArgs);
> +          sinon.assert.calledOnce(navigator.mozL10n.localize);
> +          sinon.assert.calledWith(navigator.mozL10n.localize, headerText,
> +                                 'newMessage');

For localize, I agree more, because it has a long-term effect.

::: shared/test/unit/mocks/mock_lazy_l10n.js
@@ +9,5 @@
> +
> +    get: function(callback) {
> +      setTimeout(function() {
> +        callback(MockL10n.get);
> +      });

I don't like timeouts so much because of timeout issues on Travis.

Wondering if something like "resolvedPromise.then(function() { callback(MockL10n.get); });" would work better (with "var resolvedPromise = Promise.resolve()"). Maybe not
Attachment #8439090 - Flags: review?(felash) → review-
Attachment #8439090 - Flags: feedback?(anthony) → feedback+
I was busy with a few other high-priority bugs over the past two weeks, and I had an opportunity to think about MockL10n with a fresh mind.

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #34)
> What we want to test is the external behavior. I don't care that mozL10n.get
> was called. What I care is that alert is called with a localized value,
> which happens to be return by mozL10n.get.
> 
> When the mock returns a concatenation of id + arg, we can do this in just
> one line here, whereas your changes make it necessary to have 3 lines.

I agree with you.  Returning stringified arguments achieves the same thing, but in fewer lines and also doesn't hardcode in the test the fact that mozL10n is somehow used by the code.  I've come to like this approach and consequently, I have created a new version of the patch which I'll attach shortly.


(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #35)
> Comment on attachment 8439090 [details] [diff] [review]
> Shared MockL10n and SMS tests
> 
> Review of attachment 8439090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/sms/test/unit/attachment_test.js
> @@ +500,4 @@
> >        setup(function() {
> >          this.sinon.spy(window, 'MozActivity');
> >          this.sinon.stub(window, 'alert');
> > +        this.sinon.stub(navigator.mozL10n, 'get').returnsArg(0);
> 
> seems useless with the current mock?

Yes, you have a point.  In my new patch, MockL10n's get returns stringified arguments, thus rendering this line useless.

> @@ -5801,5 @@
> >            ThreadUI.draft = null;
> >            Compose.append('some stuff');
> >            ThreadUI.recipients.add({number: '456789'});
> >  
> > -          ThreadUI.beforeEnter(transitionArgs);
> 
> why moving this?

I'm pretty sure I had a good reason to move it (related to stubbing mozL10n.localize maybe?), but in the end, I reverted these changes in the new patch because I went for a simpler approach (see below). 
> 
> @@ +5823,5 @@
> >          test('updates the header', function() {
> > +          ThreadUI.beforeEnter(transitionArgs);
> > +          sinon.assert.calledOnce(navigator.mozL10n.localize);
> > +          sinon.assert.calledWith(navigator.mozL10n.localize, headerText,
> > +                                 'newMessage');
> 
> For localize, I agree more, because it has a long-term effect.

With the localization mutation observer, I now think it's better to just test if the data-l10n-id was set correctly, instead of testing if mozL10n.localize was called.  In fact, it doesn't have to be called, the code could just as well do element.setAttribute('data-l10n-id') and the element will be correctly localized.

> 
> ::: shared/test/unit/mocks/mock_lazy_l10n.js
> @@ +9,5 @@
> > +
> > +    get: function(callback) {
> > +      setTimeout(function() {
> > +        callback(MockL10n.get);
> > +      });
> 
> I don't like timeouts so much because of timeout issues on Travis.
> 
> Wondering if something like "resolvedPromise.then(function() {
> callback(MockL10n.get); });" would work better (with "var resolvedPromise =
> Promise.resolve()"). Maybe not

AFAIK, the 'then' method will force an asynchronous invocation of the callback even if the promise is resolved (as per the A+ spec), so we wouldn't be avoiding a timeout anyways.  I left the setTimeout in the patch.
(In reply to Staś Małolepszy :stas from comment #36)
> > 
> > ::: shared/test/unit/mocks/mock_lazy_l10n.js
> > @@ +9,5 @@
> > > +
> > > +    get: function(callback) {
> > > +      setTimeout(function() {
> > > +        callback(MockL10n.get);
> > > +      });
> > 
> > I don't like timeouts so much because of timeout issues on Travis.
> > 
> > Wondering if something like "resolvedPromise.then(function() {
> > callback(MockL10n.get); });" would work better (with "var resolvedPromise =
> > Promise.resolve()"). Maybe not
> 
> AFAIK, the 'then' method will force an asynchronous invocation of the
> callback even if the promise is resolved (as per the A+ spec), so we
> wouldn't be avoiding a timeout anyways.  I left the setTimeout in the patch.

I think that "Promise.resolve().then(" is scheduling a task just after the current task is finished, so it won't trigger a timeout error. I know that "setTimeout" had a different behavior (and might compete with other "setTimeout" calls in other parts of the code), but maybe it's different nowadays for setTimeout with delay=0.

Let's keep setTimeout and if we see timeout issues we'll revisit.

(will review only tomorrow, I'm slowly reading all my mails from last week).
Comment on attachment 8447383 [details] [diff] [review]
Stringify arguments in MockL10n.get et al.

I reviewed only the SMS part.

Mostly nits, but there are also 2 failures in Travis.

(putting r- because there are 2 reviews so it's easier to see the status)
Attachment #8447383 - Flags: review?(felash) → review-
Comment on attachment 8447383 [details] [diff] [review]
Stringify arguments in MockL10n.get et al.

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

I reviewed the mock_(lazy_)l10n.js mostly.
Attachment #8447383 - Flags: review?(gandalf) → review+
Thanks, Julien, for the comments on my previous patch.  I implemented your suggestions and actually reverted a few more changes which I had previously implemented to support the stubbing idea, but which were now useless with the id+args get approach.

Thanks to this, the patch is even simpler than it was before, since most of the shraed MockL10n's API is identical to SMS's old MockL10n.

TBPL: https://tbpl.mozilla.org/?rev=2f219f5fb6b9&tree=Gaia-Try
Attachment #8447383 - Attachment is obsolete: true
Attachment #8450416 - Flags: review?(felash)
Comment on attachment 8450416 [details] [diff] [review]
Fix review nits and TBPL failures

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

Next time, please generate a patch suitable for "git am" with 8 lines of context, with "git format-patch -U8".

r=me for the SMS part.

I ran the unit tests locally and it's green, so please go ahead and merge, I don't think we need a Travis run.
Attachment #8450416 - Flags: review?(felash) → review+
(famous last words)
:)

Thanks, Julien and Gandalf.  Landed on master:

Commit: https://github.com/mozilla-b2g/gaia/commit/1c75a95bf379603340b6e703cf8c5c401bb7ddb5
Merged: https://github.com/mozilla-b2g/gaia/commit/1dc9e53393ae4680a174dffa44a958ec564ebbe8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I'd like to land some tests on v2.0 that depend on MockL10n (for example, bug 1035321). Is there any chance this could be uplifted, or should I just work around it for v2.0?
Flags: needinfo?(stas)
I wouldn't push for uplifting this whole bug to v2.0 due to the changes we made in the patch to the SMS app.  However, if you simply need to abstract MockL10n into a file in shared/test, I'd be okay with that for v2.0.
Flags: needinfo?(stas)
Actually, the changes in SMS are only in the unit tests, so as long as the tests still pass after an uplift I think it's fine.
Blocks: 1043643
Blocks: 1035385
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: