-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
blueprints/component-test: Add RFC232 variants #15934
Conversation
16ab906
to
141c386
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but generally LGTM...
@@ -11,14 +11,22 @@ module.exports = function(blueprint) { | |||
var type; | |||
|
|||
var dependencies = this.project.dependencies(); | |||
if ('ember-qunit' in dependencies || 'ember-cli-qunit' in dependencies) { | |||
if ('ember-qunit' in dependencies) { | |||
type = 'qunit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can actually be tweaked to assume "new style" can't it? Basically, there are no known (to me at least) versions of ember-qunit taht supported usage as an addon but did not support the RFC232 style...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, that was supposed to be the case, but I forgot to change it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} else if ('ember-mocha' in dependencies) { | ||
type = 'mocha-0.12'; | ||
|
||
} else if ('ember-cli-mocha' in dependencies) { | ||
var checker = new VersionChecker({ root: this.project.root }); | ||
let checker = new VersionChecker({root: this.project.root}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably just be passing in this.project
I think (not that this is a change you made)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
assert.equal(this.element.textContent.trim(), ''); | ||
|
||
// Template block usage: | ||
await(hbs` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be await render(
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
141c386
to
b3fd27c
Compare
b3fd27c
to
27b852a
Compare
|
||
test('it renders', async function(assert) { | ||
// Set any properties with this.set('myProperty', 'value'); | ||
// Handle any actions with this.set('myAction', function(val) { ... }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Handle any actions with this.set('myAction', function(val) { ... });
should this be this.on
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, because with the new system this.on
does not exist anymore
@rwjblue correct me if I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found my own answer:
https://github.com/rwjblue/ember-qunit-codemod/blob/master/__testfixtures__/ember-qunit-codemod/on.input.js
so in userland they have to define
https://github.com/rwjblue/ember-qunit-codemod/blob/master/__testfixtures__/ember-qunit-codemod/on.output.js#L9..L12
and then
this.actions.myAction = function() {}
Seems like a lot of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and it's only needed for old-style actions, with closure actions the thing I put in there is all that's needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.. 👌
|
||
test('it renders', async function(assert) { | ||
// Set any properties with this.set('myProperty', 'value'); | ||
// Handle any actions with this.set('myAction', function(val) { ... }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.on
LETS DO IT! |
Shamelessly stolen from implementation in emberjs/ember.js#15934
Addresses the
component-test
and test framework detector parts of #15933/cc @rwjblue @alexander-alvarez