Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test: add tests for compiler #721

Closed
wants to merge 1 commit into from

Conversation

nkovacs
Copy link
Contributor

@nkovacs nkovacs commented May 14, 2017

globalize-compiler complains about missing peer dependencies. One is globalize, the other two are cldr-data and iana-tz-data.

Globalize is obviously available, cldr-data is also available from bower. So it still works.

iana-tz-data was recently added to globalize-compiler, but it's not in globalize.js's master, only in the 1.3.0-alpha.3 tag. I think you might have forgotten to push to master.

@rxaviers
Copy link
Member

rxaviers commented May 17, 2017

Hi @nkovacs, this is a great addition, thank you!

Having said that, there is a detail I would prefer done differently: the tests should be explicit, not dynamically generated. Currently, the its are generated dynamically by reading ./cases data. While I understand such approach leads into smaller test file sizes and to allow generating the compiled files, but for tests it's more important they are easily readable/understood than those.

Please, could you update that? Globalize compiler tests could be used as baseline (note its functional tests) https://github.com/globalizejs/globalize-compiler

Thanks

rxaviers
rxaviers previously requested changes May 17, 2017
@@ -79,7 +79,7 @@ module.exports = function( grunt ) {
},
test: {
src: [ "test/*.js", "test/functional/**/*.js", "test/unit/**/*.js",
"!test/config.js" ],
"test/compiler/**/*.js", "!test/config.js", "!test/compiler/_compiled/**" ],
Copy link
Member

@rxaviers rxaviers May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's generate the output somewhere else outside of test/. Can we? Therefore, we don't need the negatives below for linters.

Copy link
Contributor Author

@nkovacs nkovacs May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to be inside the working directory (so it can't be os.tmpdir()), because it needs to find node_modules.

Copy link
Member

@rxaviers rxaviers May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean a sibling to test/ and node_modules/.

Copy link
Contributor Author

@nkovacs nkovacs May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why not keep everything related to these tests in the same subdirectory? In globalize-compiler's tests the fixtures are inside the tests/functional directory too.

Copy link
Member

@rxaviers rxaviers May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why not keep everything related to these tests in the same subdirectory

Because, they are generated, which means we aren't going to lint them nor to commit them. Keeping these files in a separate directory makes that easier.

In globalize-compiler's tests the fixtures are inside the tests/functional directory too.

... which I plan to fix there as well.

Copy link
Contributor Author

@nkovacs nkovacs May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's only the jshintc additions that are bothering you, another option is to use extends (assuming that works properly, I've never tried it, and it's not documented very well).
Or to switch to eslint, which also supports extending config files in subdirectories.
But I'll separate the directories for now.

Copy link
Member

@rxaviers rxaviers May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or to switch to eslint

We definitely should have moved to eslint by now... 😄

But I'll separate the directories for now.

Rethinking about it, feel free to ignore it. If you have done it already, ok; otherwise lets leave it as is.

Copy link
Member

@rxaviers rxaviers May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing test stack (grunt, AMD, phantomjs, QUnit) is something I wanted to get updated to using npm scripts, mocha only. So, your tests are closer to that.

Copy link
Contributor Author

@nkovacs nkovacs May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will putting the generated files into tmp/ affect the build/publish process? That's where grunt-contrib-uglify puts the minified files, but it appears they are not used after that.

Copy link
Member

@rxaviers rxaviers May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the build process finishes, tmp/.build/ files are saved into dist/ and tmp/ is left/abandoned.

"module": false,
"__dirname": false,
"describe": false,
"it": false,
Copy link
Member

@rxaviers rxaviers May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move your just created test/compiler into test-compiler. That way we isolate the existing tests that use QUnit and your just created ones that use mocha.

@nkovacs
Copy link
Contributor Author

nkovacs commented May 17, 2017

The problem is that when the runtime bundle is compiled, it needs to be executed with the exact same formatters. That's why I need the separate case files.

Some pseudocode:

describe("The compiled `basic.js`", function() {
	it("should include formatDate", function() {
		var formatter = Globalize.dateFormatter({date: "medium"});
		var expected = formatter(new Date(2017, 3, 15));

		compiled = compile(formatter); // This is just the runtime bundle
		// Write compiled to a file.

		// Now I need to generate a file that requires the compiled bundle and contains
		// console.log(Globalize.dateFormatter({date: "medium"})(new Date(2017, 3, 15)));
		// How do I do that without duplicating everything?

		// Then run this file and get its output, put it into actual.

		assert.equal( actual, expected );
	});
});

The only thing I can think of here is to put the formatter part into a function, and use function.toString() to generate the testfile.
E.g.:

describe("The compiled `basic.js`", function() {
	it("should include formatDate", function() {
		var formatter;
		var testFunction = function() {
			formatter = Globalize.dateFormatter({date: "medium"});
			return formatter(new Date(2017, 3, 15));
		};
		var expected = testFunction();

		compiled = compile(formatter); // This is just the runtime bundle
		// Write compiled to a file.

		var code = "require('_tmp/compiled.js'); var testFunction = " +
			testFunction().toString() +
			"; console.log(testFunction)";

		// Write code to a file.
		// Then run this file and get its output, put it into actual.

		assert.equal( actual, expected );
	});
});

I prefer the current code because you have the different test cases clearly listed in the testcase files without too much overhead.

@rxaviers
Copy link
Member

rxaviers commented May 17, 2017

The problem is that when the runtime bundle is compiled, it needs to be executed with the exact same formatters...

The compiled code can be generated before the tests are executed, which is the approach taken in globalize-compiler tests: https://github.com/globalizejs/globalize-compiler

@nkovacs
Copy link
Contributor Author

nkovacs commented May 18, 2017

In globalize-compiler the fixtures are generated before the tests run. This means that technically you are not actually testing the compiler, you're just testing the runtime code, and assuming that the bundle is always generated correctly. You don't get proper code coverage, you can't use watch mode, and you can't run mocha directly either.

In globalize-compiler's functional tests, the only reason this test works, for example, is that the fixture contains a completely unrelated Globalize.formatDate call. This dependency is not obvious and easy to break, especially since the functional tests use the fixtures of the unit tests, it's not obvious that those fixtures are needed by the functional tests.

The tests in globalize-compiler also validate the exact output. The tests here do not. They only check that the testcase has the exact same output when running with the runtime bundle as when running with the full fat globalize.js + cldr data, because that's what you care about when testing the compiler. You already have tests that verify that the individual formatters do what they're supposed to do, there's no need to check that here too, and it makes maintenance simpler.

While I would prefer to be able to have explicit its instead of dynamically generating them, there's nothing wrong with doing that. Pugjs does something similar, and I used that as inspiration.
While you lose some clarity in test.js, you gain that in the cases/*.js files, since you have all the formatters that are being tested in a reasonably readable form, with no duplication.

@rxaviers
Copy link
Member

rxaviers commented May 18, 2017

You have valid points! 👍 cc @jzaefferer

files.forEach( function(file) {
var name = path.basename( file, ".js" );
describe( name, function() {
it( "should return identical data", function( done ) {
Copy link
Member

@rxaviers rxaviers May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should return identical output compared to normal globalize? Can we update the description

Copy link
Contributor Author

@nkovacs nkovacs May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't have time to work on this. Do you still want me to do this? Or you can just commit it directly to master.

Copy link
Member

@rxaviers rxaviers May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, this can be done later.

@rxaviers rxaviers dismissed their stale review May 19, 2017

Re-evaluating it

@rxaviers rxaviers added this to the 1.3.0 milestone May 19, 2017
@rxaviers
Copy link
Member

rxaviers commented May 19, 2017

Merged! Thank you very much @nkovacs. This is a great already and improvements could be done later.

@rxaviers rxaviers closed this in 9389955 May 19, 2017
rxaviers added a commit to rxaviers/globalize that referenced this issue May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants