-
Notifications
You must be signed in to change notification settings - Fork 12k
feat(command): ng test command runs karma #86
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
Conversation
}, | ||
|
||
run: function(commandOptions, rawArgs) { | ||
var promise = new Promise(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.
What about having this.server.start()
inside this function?
var promise = new Promise(function() {
return this.server.start();
});
That works update coming soon. Been doing a whole bunch of TypeScript recently, so it didn't occur to me that lexical scoping via fat arrow wasn't in play in your snippet, so it had to be tweaked to:
I've opted to go w/ |
|
||
init: function(){ | ||
var cwd = process.cwd(); | ||
this.karma = this.karma || require(cwd + '/node_modules/karma/lib/index'); |
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.
use require.resolve
instead
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.
@IgorMinar I'm not familiar w/ require.resolve
are you suggesting that I use do this:
this.karma = this.karma || require(require.resolve(cwd + '/node_modules/karma/lib/index'));
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 that's not right. the problem is that we can't be hardcoding paths because it will cause issues on windows.
why can't we just say require('karma')
? wouldn't that resolve correctly?
cwd
points to the application dir and this file should be executed from cwd
+ '/node_modules/angular-cli/addon/ng2/commands/test.jsno? If that's the case then
require('karma')` should just work...
shouldn't we trigger otherwise this looks good, I think. @cironunes can you take a look as well? @cironunes, @Brocco can you open another issue to integrate the build pipeline with karma so that we automatically rebuild the project when |
@Brocco, nice one. As Igor said, please just make sure to run @IgorMinar, created the issue #87 |
ng test will now execute a build and kick off karma |
return ng([ | ||
'test', | ||
'--skip-npm', | ||
'--skip-bower' |
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.
do we need these two skip flags? I think they are needed only for init
and new
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 went back and checked ember's implementation and couldn't find a test spec for the test command. I think it is because we would need to run an npm install... otherwise requiring karma will fail
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.
but ng init
is executed above with these flags. I don't thing we need the flags here. they don't do anything. try removing.
Protractor integration is getting done: #94. Will we integrate it on We'll probably need some flags as Igor were saying |
I suggest we commit this first and then add protractor in a follow up PR wrt I'm open to be convinced otherwise. But again, we can discuss that in a follow up PR |
@Brocco are you blocked on anything? If not, can you resolve the comments above please so that we can merge this in? |
@IgorMinar I'm still not sure how to go about testing the test command ( I updated this branch/PR to exclude the running of the ng test acceptance tests ( I added the karma args and defaulted it to be a single-run to alleviate confusion "why's that still running" |
Updated the readme to update the documentation on how to execute tests |
@Brocco can you rebase this with the latest changes from master? |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Signed.
|
I signed it!
|
@Brocco have you updated the PR? I'm still seeing:
|
9cf843d
to
871fcc5
Compare
5443097
to
07ef9f2
Compare
@@ -14,6 +15,8 @@ module.exports = function(config) { | |||
{pattern: 'node_modules/angular2/bundles/http.dev.js', included: true, watched: true}, | |||
{pattern: 'node_modules/angular2/bundles/router.dev.js', included: true, watched: true}, | |||
{pattern: 'node_modules/angular2/bundles/testing.dev.js', included: true, watched: true}, | |||
{pattern: 'node_modules/es6-shim/es6-shim.js', included: true, watched: true}, |
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 these be in the same order as https://github.com/angular/angular-cli/blob/master/lib/broccoli/angular2-app.js#L21-L30 ?
Other than the comments I added in, lgtm. |
Overrode the ember-cli test command to initialize the karma server Then start the karma server based upon the karma.conf.js config file closes angular#70
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Overrode the ember-cli test command to initialize the karma server
Then start the karma server based upon the karma.conf.js config file
closes #70