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

Fix for test name to contain parent suite(s) names #85

Merged
merged 1 commit into from
Mar 10, 2016

Conversation

phillipj
Copy link
Contributor

@phillipj phillipj commented Mar 7, 2016

This fixes issue reported in #62 where test names doesn't include parent test suites in the generated report.

Also created minimal unit test setup.

Any thoughts?

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

@phillipj
Copy link
Contributor Author

phillipj commented Mar 7, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@phillipj phillipj force-pushed the fix/default-test-name branch from fe69381 to 0c6fd02 Compare March 7, 2016 21:25
"eslint": "^1.2.1",
"eslint-config-standard": "^4.1.0",
"eslint-plugin-standard": "^1.3.1",
"grunt": "^0.4.1",
"grunt-bump": "^0.5.0",
"grunt-cli": "^0.1.13",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed locally only globally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added grunt-cli to avoid requiring global install of gulp. As good npm citizens we should not require such global installs by developers trying to run the tests :)

@dignifiedquire
Copy link
Member

Two small nitpicks other than that thanks a lot!

If you feel up for it path.isAbsolute is failing on the CI as it's not available on node 0.10, so would be great to add fix for that as well so everything is green.

@phillipj phillipj force-pushed the fix/default-test-name branch 2 times, most recently from c229a79 to 73d49e0 Compare March 7, 2016 21:38
@phillipj
Copy link
Contributor Author

phillipj commented Mar 7, 2016

Thanks for lightning quick response on this @dignifiedquire 👍 Pushed an updated to handle path.isAbsolute() issues.

Does not require developers to have grunt installed globally sound reasonable to you? IMO there's no benefits of installing globally, as opposed to having self-contained modules standing for their own dependencies.

@dignifiedquire
Copy link
Member

Does not require developers to have grunt installed globally sound reasonable to you? IMO there's no benefits of installing globally, as opposed to having self-contained modules standing for their own dependencies.

Well there is the big benefit of not having to run ./node_modules/.bin/grunt but rather being able to run grunt. Also if you have like me about 30 karma repos that all have a local grunt-cli installed vs one global grunt-cli it takes up less space :) grunt-cli itself is also just a tiny wrapper calling the actual local grunt installation in each path from where it's called and specifically prefers to be installed globally (See the readme here https://github.com/gruntjs/grunt-cli).

@phillipj
Copy link
Contributor Author

phillipj commented Mar 9, 2016

Well there is the big benefit of not having to run ./node_modules/.bin/grunt but rather being able to run grunt. Also if you have like me about 30 karma repos that all have a local grunt-cli installed vs one global grunt-cli it takes up less space :)

You might still install and use grunt globally as you normally would.

grunt-cli itself is also just a tiny wrapper

Might be worth noting this really has nothing to do with grunt in particular.

This article describes much of my thoughts on this topic: https://www.smashingmagazine.com/2016/01/issue-with-global-node-npm-packages/

If you'd still prefer global installation of grunt as the only option, just say the word and I'll revert those changes.

P.S. pushed an update cause the it(..) description was more or less missing.

@dignifiedquire
Copy link
Member

I'm okay with leaving it in here I think but please add a npm script for running it then, so we go all the way.

@phillipj
Copy link
Contributor Author

phillipj commented Mar 9, 2016

Cool.

It's already in there. Running npm test will run the default grunt task. Or did you have another task in mind?

@dignifiedquire
Copy link
Member

Ah right, I missed that. Looks good then. Can you change the commit message to follow our convention please? then I can merge it

@phillipj
Copy link
Contributor Author

phillipj commented Mar 9, 2016

Wooops, sorry about that. I'll try to get that fixed before the end of the day.

This fixes issue reported in karma-runner#62 where test names doesn't include parent test suites in the generated report.

Also created minimal unit test setup.
@phillipj phillipj force-pushed the fix/default-test-name branch from 29ca556 to 8e6e202 Compare March 9, 2016 21:28
@phillipj
Copy link
Contributor Author

phillipj commented Mar 9, 2016

Revised the commit message.

If I'm to be really strict about the subsystem of the commit, I should split it up into several commits though... Would you prefer multiple commits here @dignifiedquire, or would you consider it fine as is?

dignifiedquire added a commit that referenced this pull request Mar 10, 2016
Fix for test name to contain parent suite(s) names
@dignifiedquire dignifiedquire merged commit af6c11f into karma-runner:master Mar 10, 2016
@dignifiedquire
Copy link
Member

It's okay :) let's get this merged :) Thanks :octocat:

@phillipj
Copy link
Contributor Author

Hooray! Thanks for all your hard work maintaning these modules 👍

On Thursday, 10 March 2016, Friedel Ziegelmayer [email protected]
wrote:

It's okay :) let's get this merged :) Thanks [image: :octocat:]


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

@dignifiedquire
Copy link
Member

Released in 0.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants