- Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addon/ng2/commands/test.js Outdated
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?
varpromise=newPromise(function(){returnthis.server.start();});Brocco commented Dec 3, 2015
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: varserver=this.server;returnnewPromise(function(){returnserver.start();});// orvarself=this;// orreturnnewPromise(function(){returnthis.server.start();}.bind(this));I've opted to go w/ |
addon/ng2/commands/test.js Outdated
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 thenrequire('karma')` should just work...
IgorMinar commented Dec 4, 2015
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 |
cironunes commented Dec 4, 2015
@Brocco, nice one. As Igor said, please just make sure to run @IgorMinar, created the issue #87 |
Brocco commented Dec 5, 2015
ng test will now execute a build and kick off karma |
tests/acceptance/test.spec.js Outdated
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.
cironunes commented Dec 7, 2015
Protractor integration is getting done: #94. Will we integrate it on We'll probably need some flags as Igor were saying |
IgorMinar commented Dec 10, 2015
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 |
IgorMinar commented Dec 10, 2015
@Brocco are you blocked on anything? If not, can you resolve the comments above please so that we can merge this in? |
Brocco commented Dec 10, 2015
@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" |
Brocco commented Dec 10, 2015
Updated the readme to update the documentation on how to execute tests |
cironunes commented Jan 22, 2016
@Brocco can you rebase this with the latest changes from master? |
googlebot commented Jan 24, 2016
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.
|
jkuri commented Jan 24, 2016
Signed.
|
Brocco commented Jan 24, 2016
I signed it!
|
cironunes commented Jan 24, 2016
@Brocco have you updated the PR? I'm still seeing:
|
9cf843d to 871fcc5Compare5443097 to 07ef9f2Compare| {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 ?
filipesilva commented Feb 14, 2016
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 closesangular#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