Skip to content
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

feat(command): ng test command runs karma #86

Merged
merged 1 commit into from Feb 14, 2016

Conversation

Brocco
Copy link
Contributor

@Brocco Brocco commented Dec 3, 2015

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

},

run: function(commandOptions, rawArgs) {
var promise = new Promise(function(){});
Copy link
Member

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();
});

@Brocco
Copy link
Contributor Author

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:

var server = this.server;

return new Promise(function(){
  return server.start();
});
// or
var self = this;
// or
return new Promise(function(){
  return this.server.start();
}.bind(this));

I've opted to go w/ .bind


init: function(){
var cwd = process.cwd();
this.karma = this.karma || require(cwd + '/node_modules/karma/lib/index');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use require.resolve instead

Copy link
Contributor Author

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'));

Copy link
Contributor

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
Copy link
Contributor

shouldn't we trigger ng build when running ng test?

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 src changes, which then updates dist. we should then tell karma to rerun the tests. angular/angular repo has this setup. the implementation is a bit messy but it's a good start.

@cironunes
Copy link
Member

@Brocco, nice one. As Igor said, please just make sure to run ng build before starting karma.

@IgorMinar, created the issue #87

@Brocco
Copy link
Contributor Author

Brocco commented Dec 5, 2015

ng test will now execute a build and kick off karma

return ng([
'test',
'--skip-npm',
'--skip-bower'
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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
Copy link
Member

Protractor integration is getting done: #94.

Will we integrate it on ng test in this PR or do you want me to create a separate issue so we can discuss?

We'll probably need some flags as Igor were saying ng test --all(default)/unit/e2e

@IgorMinar
Copy link
Contributor

I suggest we commit this first and then add protractor in a follow up PR

wrt ng test running protractor tests - I dunno, historically we've always treated e2e tests differently because they are slow and people rarely run them locally (only if CI is broken). So I think that we should make it easy to run e2e tests locally, but don't run them by default.

I'm open to be convinced otherwise. But again, we can discuss that in a follow up PR

@IgorMinar
Copy link
Contributor

@Brocco are you blocked on anything? If not, can you resolve the comments above please so that we can merge this in?

@Brocco
Copy link
Contributor Author

Brocco commented Dec 10, 2015

@IgorMinar I'm still not sure how to go about testing the test command (npm test from the cli project)

I updated this branch/PR to exclude the running of the ng test acceptance tests (xdescribe) in case anyone else wants to take a look in the meantime.

I added the karma args and defaulted it to be a single-run to alleviate confusion "why's that still running"

@Brocco
Copy link
Contributor Author

Brocco commented Dec 10, 2015

Updated the readme to update the documentation on how to execute tests

@cironunes
Copy link
Member

@Brocco can you rebase this with the latest changes from master?

@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jkuri
Copy link
Contributor

jkuri commented Jan 24, 2016

Signed.

On 24 Jan 2016, at 03:17, googlebot notifications@github.com wrote:

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/ https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data https://cla.developers.google.com/clas and verify that your email is set on your git commits https://help.github.com/articles/setting-your-email-in-git/.
If you signed the CLA as a corporation, please let us know the company's name.

Reply to this email directly or view it on GitHub #86 (comment).

@Brocco
Copy link
Contributor Author

Brocco commented Jan 24, 2016

I signed it!
On Jan 23, 2016 9:17 PM, "googlebot" notifications@github.com wrote:

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).

[image: 📝] Please visit https://cla.developers.google.com/
https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll

verify. Thanks.


Reply to this email directly or view it on GitHub
#86 (comment).

@cironunes
Copy link
Member

@Brocco have you updated the PR?

I'm still seeing:

This branch is out-of-date with the base branch
Merge the latest changes from master into this branch.

@Brocco Brocco force-pushed the ng-test branch 6 times, most recently from 9cf843d to 871fcc5 Compare February 6, 2016 04:51
@Brocco Brocco force-pushed the ng-test branch 7 times, most recently from 5443097 to 07ef9f2 Compare February 14, 2016 04:34
@Brocco Brocco assigned hansl and unassigned cironunes Feb 14, 2016
@@ -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},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filipesilva
Copy link
Contributor

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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(command): ng test
7 participants