Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Description • 10 years ago

We have a bunch of tests for l10n.js that we're landing in bug 914414.

The tests live in https://github.com/l20n/l20n.js/tree/gaia/tests

We want to port them to Gaia's testing suite and land in apps/gallery/tests/unit/l10n.

We may need some Gaia Team's help with that.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 1 • 10 years ago

Initial bunch of tests for parser, compiler and integration.

I'm also moving l10n tests to l10n directory.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 2 • 10 years ago

Comment on attachment 8401304 [details] [diff] [review]
l10n-tests.diff

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

Mostly because those tests would break on tbpl. Let's fix that and we will be good to go.

::: apps/gallery/test/unit/l10n/integration/basic_test.js
@@ +1,1 @@
> +if (typeof navigator !== 'undefined') {

For this file and all the others you need to |require('/shared/js/l10n.js');|. Each test files is supposed to runs into its own sandbox. On Travis there is a bug which is beeing worked on at the moment where this is not the case, so you are lucky because of the order of the test files in this folder, but it will break on TBPL.

Also and that's just for this file, it's unclear to me why it lives into integration ?

@@ +28,5 @@
> +
> +  it('should return the string value of brandName', function() {
> +    var value = ctx.get('brandName');
> +    assert.strictEqual(value, 'Firefox');
> +  });

For this file and all the others can you add a line break between each test command (or it in your case).

::: apps/gallery/test/unit/l10n/lib/context/fixtures/i-default.properties
@@ +1,1 @@
> +foo=Foo i

What is the meaning of the name of the file: i-default ?

::: apps/gallery/test/unit/l10n/lib/context/resources_test.js
@@ +53,5 @@
> +  it('should get ready', function(done) {
> +    whenReady(ctx, done);
> +    ctx.requestLocales();
> +  });
> +});

Can you add an explicit assert instead of relying on the fact that the code does not throw (I think that's what you are doing).

::: apps/gallery/test/unit/l10n/lib/parser_test.js
@@ +61,5 @@
> +        parse(strings[i]);
> +      } catch (e) {
> +        errorsThrown += 1;
> +      }
> +    } 

nit: whitespaces

@@ +77,5 @@
> +    ];
> +    for (var i in strings) {
> +      var ast = parse(strings[i]);
> +      assert.equal(Object.keys(ast).length, 0);
> +    }  

nit: whitespaces

	Staś Małolepszy :stas
	Comment 3 • 10 years ago

Vivien,

Here's an updated patch with 47 tests for l10n.js.  It's part of the patch for bug 914414, so if you r+ this one, I'll land both in a single merge.  Thanks!

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #2)
> Mostly because those tests would break on tbpl. Let's fix that and we will
> be good to go.
> 
> ::: apps/gallery/test/unit/l10n/integration/basic_test.js
> @@ +1,1 @@
> > +if (typeof navigator !== 'undefined') {
> 
> For this file and all the others you need to
> |require('/shared/js/l10n.js');|. Each test files is supposed to runs into
> its own sandbox. On Travis there is a bug which is beeing worked on at the
> moment where this is not the case, so you are lucky because of the order of
> the test files in this folder, but it will break on TBPL.

Am I correct thinking that it's enough to |require('/shared/js/l10n.js');| in setup.js?  Both Travis and TBPL seem to read that file via Test Agent.  See the following builds:

https://travis-ci.org/mozilla-b2g/gaia/builds/22498067
https://tbpl.mozilla.org/?tree=Try&rev=915a45e903da

> Also and that's just for this file, it's unclear to me why it lives into
> integration ?

An artifact from our repo, where 'integration' tests were supposed to test general use-cases of the localization context, instead of its specific components.  This is not needed and might be confusing to Gaia developers, so we renamed the file so that it lives under lib/context.

> 
> @@ +28,5 @@
> > +
> > +  it('should return the string value of brandName', function() {
> > +    var value = ctx.get('brandName');
> > +    assert.strictEqual(value, 'Firefox');
> > +  });
> 
> For this file and all the others can you add a line break between each test
> command (or it in your case).

Done.

> 
> ::: apps/gallery/test/unit/l10n/lib/context/fixtures/i-default.properties
> @@ +1,1 @@
> > +foo=Foo i
> 
> What is the meaning of the name of the file: i-default ?

I removed that file, good catch.  We use i-default in L20n master to mean a locale without an explicit locale code, as per the RFC 2277.  This isn't used anywhere in l10n.js for now, so the file is not needed.

> 
> ::: apps/gallery/test/unit/l10n/lib/context/resources_test.js
> @@ +53,5 @@
> > +  it('should get ready', function(done) {
> > +    whenReady(ctx, done);
> > +    ctx.requestLocales();
> > +  });
> > +});
> 
> Can you add an explicit assert instead of relying on the fact that the code
> does not throw (I think that's what you are doing).

Done, here and in a few other places.

> nit: whitespaces

We made all new test files pass jshint tests in Gaia :)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 4 • 10 years ago

(In reply to Staś Małolepszy :stas from comment #3)
> > ::: apps/gallery/test/unit/l10n/integration/basic_test.js
> > @@ +1,1 @@
> > > +if (typeof navigator !== 'undefined') {
> > 
> > For this file and all the others you need to
> > |require('/shared/js/l10n.js');|. Each test files is supposed to runs into
> > its own sandbox. On Travis there is a bug which is beeing worked on at the
> > moment where this is not the case, so you are lucky because of the order of
> > the test files in this folder, but it will break on TBPL.
> 
> Am I correct thinking that it's enough to |require('/shared/js/l10n.js');|
> in setup.js?  Both Travis and TBPL seem to read that file via Test Agent. 
> See the following builds:
> 

Yes you are. I missed the code from setup.js :)

	Staś Małolepszy :stas
	Comment 5 • 10 years ago

Thanks, Vivien.

This landed as part of the patch in bug 914414:

Commit: https://github.com/mozilla-b2g/gaia/commit/fd2d573db38fa4ff5e607e62a35a23cb15c025d7
Merged: https://github.com/mozilla-b2g/gaia/commit/89691d2d1a98aef25f3937fbe995413a5d943abe