generateTestFile() can generate broken code

[Turbo87]

generateTestFile() is not escaping the moduleName, errorMessage and other strings, which can lead to broken code. right now it is the responsibility of the calling code to escape the strings properly, but it really shouldn't be. any ideas how we can change the responsibility without breaking the world?

/cc @rwjblue @stefanpenner @trentmwillis

[kellyselden]

We could make a new function generateEscapedTestFile, and gradually move implementers over to the new one.

[Turbo87]

Maybe we can do something like this.project.generateTestFile.escapesInput = true and (temporarily) check for that flag in eslint, jshint, jscs, ...

[kellyselden]

That works too!

[trentmwillis]

I'm not actually convinced that having generateTestFile escape the input is correct.

If the generateTestFile is a hook, then that means whoever is calling the hook is responsible for the information being provided. Thus, if the intent is that the information should always be properly escaped then it makes more sense that the caller is the one to escape it.

In other words, implementing the escaping once in each caller is annoying for sure, but it is better than making anyone who overrides the generateTestFile hook have to implement it, because conceivably the hook (being a hook), will be defined more frequently than the caller of said hook.

This gets confusing though since the caller is, in a sense, also a hook as it can be invoked by multiple different addons. So I think if we actually want neither party to go through the laborious process of defining escaping logic, then we should upstream it into Ember-CLI (since the project is the commonality between the two parties). This would likely mean that we need a wrapper around invoking generateTestFile.

[trentmwillis]

@Turbo87 what should we do here? Has this come up as an actual issue for someone?

[Turbo87]

@trentmwillis at this point I'm no longer sure if the generateTestFile() method was such a great idea... for ESLint I'm thinking about changing ember-cli-eslint again to not rely on it and generate tests by itself. this would also allow us to generate a single test file/module with multiple tests instead of having a test module per tested file. if it proves valuable we could also extract those test generators out of the addon afterwards.

[trentmwillis]

I'm unsure I see the value in doing that. I think it makes sense for ember-cli-qunit and ember-cli-mocha to essentially serve as "plugins" for ember-cli-eslint so that way it doesn't need to know about specific test frameworks.

As for generating one file, couldn't that be done with the existing implementation? Seems like a reduce using the string output of generateTestFile() could be written a single file by ember-cli-eslint.

[Turbo87]

I think it makes sense for ember-cli-qunit and ember-cli-mocha to essentially serve as "plugins" for ember-cli-eslint so that way it doesn't need to know about specific test frameworks.

yeah, I thought so at first too, but it's a little more complicated because we need to deal with both ember-cli-eslint and broccoli-lint-eslint (or the same combos for JSHint, JSCS, TemplateLint, ...). The ember-cli-* plugins are allowed to know about Ember CLI and the testing frameworks, but the Broccoli plugins shouldn't. Yet the Broccoli plugins are ultimately the ones that need to generate the tests, and depending on how those tests should look like you will need different kinds of test generators (one file with one module with multiple tests, multiple files with one module and one test each, ...). Right now the test generators that ember-cli-qunit and ember-cli-mocha provide don't allow for that, because I didn't design them with those constraints in mind 😞 . Relying on those generators obviously also makes it harder to refactor the addons/plugins because they are not really in control over what they actually generate.

couldn't that be done with the existing implementation? Seems like a reduce using the string output of generateTestFile() could be written a single file by ember-cli-eslint.

currently the test generators are generating a test inside a module in each file. if you concat those files you will have a single file but still have one module for each original file. what I would like to have instead is have a single module with one test for each of the files. we could obviously parse the generated JS and extract the test code from the module, but I think it would be much easier to just generate it properly in the first place.

[Turbo87]

ember-cli-eslint and ember-cli-template-lint are using https://github.com/ember-cli/aot-test-generators now instead of the generateTestFile() method so this is no longer an issue