Add test module grouping by default #243
Conversation
| ].join('\n')); | ||
| })); | ||
|
|
||
| it('generates a QUnit test file if "testGenerator: qunit" and "group: foo" are provided', co.wrap(function *() { |
There was a problem hiding this comment.
Just to be clear, it isn't possible for an end user to do this. That is intentional right?
There was a problem hiding this comment.
as an end user you can only group: true/false and manually override the testGenerator option, but the defaults should work fine for everyone. this is similar to what we have in ember-cli-eslint now.
| }] | ||
| ); | ||
| let output = ''; | ||
| if (this._testGenerator) { |
There was a problem hiding this comment.
Do we properly support this anyways? If you have a project today without ember-cli-qunit or ember-cli-mocha (prior to this PR) what happens? What happens after this change?
There was a problem hiding this comment.
If you have a project today without ember-cli-qunit or ember-cli-mocha (prior to this PR) what happens?
Ember CLI will display a warning that you should install a test framework (for every generated test 😞) and it will generate an empty file
What happens after this change?
ember-cli-template-lint will display a warning that it couldn't detect a test framework (once) and it will generate an empty file
broccoli-template-linter.js
Outdated
| ); | ||
| let output = ''; | ||
| if (this._testGenerator) { | ||
| if (this.options.group) { |
There was a problem hiding this comment.
This is for explicitly opting out via group :false?
| output = [ | ||
| this._testGenerator.suiteHeader(`TemplateLint | ${relativePath}`), | ||
| this._testGenerator.test('should pass TemplateLint', passed, | ||
| `${relativePath} should pass TemplateLint.\n\n${errorDisplay}`), |
There was a problem hiding this comment.
the errorDisplay is escaped by aot-test-generators?
There was a problem hiding this comment.
yes, aot-test-generators handles all escaping now, and actually with far less bugs than js-escape-string did before...
index.js
Outdated
| annotation: 'TemplateLinter', | ||
| templatercPath: this.project.root + '/.template-lintrc', | ||
| generateTestFile: this.project.generateTestFile, | ||
| testGenerator: this._options.testGenerator, | ||
| group: (this._options.group !== false) ? type : undefined, |
There was a problem hiding this comment.
This makes it only possible for end users of ember-cli-template-lint to disable grouping, not to add a custom group. Is that intended? Based on one of the tests, it seemed that you did mean to allow this to be specified as a string...
There was a problem hiding this comment.
there is a difference between the group option of the broccoli plugin and the group option of the Ember CLI addon. the former accepts a string to name the group, the latter only accepts a boolean to enable/disable grouping in general.
There was a problem hiding this comment.
I don't find it valuable to have two concepts for the same word here. The broccoli plugin only exists to support the addon....
There was a problem hiding this comment.
In other words, we should make both allow strings or both only care about booleans.
There was a problem hiding this comment.
well, they have different meanings though. the CLI addons is only configured once, while the broccoli plugin could potentially run for multiple lint trees. in ESLint that results in ESLint | app, ESLint | tests, and for some even ESLint | mirage, and the same might be true here although from initial testing it seems that TemplateLint | templates is the majority of it
There was a problem hiding this comment.
@rwjblue I've renamed the option in the Broccoli plugin to groupName to better reflect that they expect different types. I'm also excluding the groupName from the test suite name if it equals templates which seems to be the default in most Ember projects.
|
looks like CI failures are related to changes here |
... to differentiate between "group" (boolean) and "groupName" (string)
|
@rwjblue forgot to adjust the test code to the new option name 🙈 should be 🍏 now... |
similar to ember-cli/ember-cli-eslint#176
/cc @rwjblue