-
Notifications
You must be signed in to change notification settings - Fork 1.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
ember test
with Testem.
#388
Conversation
This is working now, but I still need to get test passing, plus figure it out a better way to proxy testem options. welcome any comments. |
<3 |
|
||
return build.run(ui, { environment: 'test', outputPath: options.cwd }). | ||
then(function() { | ||
test.run(ui, options); |
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.
return
@stefanpenner did some changes and took some of the suggestions made on previous version, I feel comfortable with this as a 0.1 for There is still one issue, tests are actually not being run, can you give me some review on that, I think I'm doing something wrong with le promises. |
var testRun = env.tasks.test.run; | ||
|
||
command.run(ui, env, { }).then(function(){ | ||
assert.equal(buildRun.called, 2, 'expected build task to be called once'); |
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 is intentional just to verify that my assertions are not being hit :(
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.
wanna leave an inline comment?
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.
@stefanpenner sure, but this "should" be running? Or I did something wrong with the promises?
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.
@stefanpenner added comment, having a second look trying to figure it out what's up with this. Also will start to work on docs for the website.
@abuiles I will review and investigate the issue you are having first thing in the morning. |
thanks 👍 |
|
||
command.run(ui, env, { }).then(function(){ | ||
// Intentional just to verify that assertions is not hit. | ||
assert.equal(buildRun.called, 2, 'expected build task to be called once'); |
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.
@MajorBreakfast could you chime in here? I was expecting this tests to assert but they are not, must be something with the promises.
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.
you forgot the return statement in line 41.
BTW move the .then()
to the next line. -> ARCHITECTURE.md style guide.
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.
@MajorBreakfast > you forgot the return statement in line 41.
Facepalm... thanks!
All fixed here, tests are now running (thanks @MajorBreakfast!) I'll call this a 0.1 🎆 |
run: function() { } | ||
}, | ||
test: { | ||
run: function() { } |
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.
inherit from real Task
class
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.
function() { return Promise.resolve() }
, then you don't need to stub it. (Also omit true
, it's not 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.
fixed the return value, not inhering for the real Task
, this is a stub it's fine to use just an object.
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.
@abuiles If you want you can take a "comment" revenge next time I post a PR :) |
@MajorBreakfast it's fine ;) I'll go over this again later tonight. |
@MajorBreakfast just went over the second revision and make changes accordingly, thanks! cc @stefanpenner |
@@ -0,0 +1,61 @@ | |||
'use strict'; |
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.
love the file name
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.
:D
i think this a good first pass, thanks @abuiles |
`ember test` with Testem.
Not ready to be merged yet, but would love feedback as we advance.
Things to do
In the future it would be great to make easy for people to extend/patch task so in the
test
case everyone could write their own on top of their favorite runner and just installed them from bower.