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

Karma fixes for latest versions #2058

Merged
merged 1 commit into from Feb 5, 2016
Merged

Conversation

eiriksm
Copy link
Member

@eiriksm eiriksm commented Feb 4, 2016

So, here is some fixes that closes #2035 and #2044 and #2056

First off, one of the issues were that we are testing across 3 versions of npm. They have different behaviours when encountering peerDependencies. So ideally I would almost say we should upgrade npm to at least npm@2 for node 0.10. But open to opinions there. This was the least disruptive change, in my opinion.

Second, one of the issues were pointing out that the response object changed for one of the tests. So I ran the same test code with node, where the result was illuminating the fact that the browser test was testing for something only happening in the browser. So I changed that test, so it can check for a result that also is the same result when running on node.

Fixes #2035
Fixes #2044
Closes #2056

@eiriksm eiriksm mentioned this pull request Feb 4, 2016
@eiriksm
Copy link
Member Author

eiriksm commented Feb 4, 2016

Also, let me know if I should squash these commits. Had to do a couple of runs to get the suite to run it on travis to be sure :)

@simov
Copy link
Member

simov commented Feb 4, 2016

Your commits are great for explaining what's going on. I often find myself scrolling through the last commits for a project, so from a usability point of view clicking on a single and more meaningful commit there is easier. Of course this can be approach in many different ways, but in this case one commit might be better.

Also see how I'm using the Fixes and Closes to actually close those PRs and Issues once this gets merged in. Probably another syntax might work too, but I don't think GitHub is smart enough to link those from:

So, here is some fixes that closes #2035 and #2044 and #2056

Thanks for taking the time to work on this 👍

@eiriksm
Copy link
Member Author

eiriksm commented Feb 5, 2016

Also see how I'm using the Fixes and Closes to actually close those PRs and Issues once this gets merged in

I am aware of the syntax :) Just wanted to make sure the issues were being referenced, for easier navigation.

Updated the branch and used this to indicate what is being closed and fixed.

simov added a commit that referenced this pull request Feb 5, 2016
Karma fixes for latest versions
@simov simov merged commit 176f3a8 into request:master Feb 5, 2016
@simov
Copy link
Member

simov commented Feb 5, 2016

Thanks 🐈

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.

Karma tests fail
2 participants