Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

ES6 coverage, updated karma and jasmine dependencies #519

Merged
merged 1 commit into from
Sep 30, 2016

Conversation

bellstrand
Copy link
Contributor

Added ES6 coverage.
The old setup showed coverage on the transpiled code which makes the coverage percentage show number that does not represent the reality and it's hard to track what you need to test.

The new setup uses babel-plugin-__coverage__ instread of isparta & istanbul.

Isparta is since 27 March unmaintained, so I think it's time to swap it out for something new.

Updated dependencies for karma & jasmine when I were at it.

Any thought on this? Feedback is always welcome!

@EisenbergEffect
Copy link
Contributor

@bellstrand Would you mind rebasing on master?

@AshleyGrant
Copy link
Collaborator

@bellstrand just in case you haven't rebased off the upstream master before, here's a tutorial I created last year when I learned how to do this: https://www.youtube.com/watch?v=M7ZYkjOWr6g

Here's a summary:
The git commands you'll need as the fork developer are:
Add the upstream repo as a remote
git remote add upstream upstreamrepourl

Checkout your master branch:
git checkout master

Pull using rebase the upstream master branch to your fork's master branch:
git pull --rebase upstream master

Checkout your feature branch:
git checkout featurebranch

Rebase the feature branch:
git rebase master

If there are conflicting changes, git will tell you to merge any conflicting changes. Merge these changes in your text editor and then continue the rebase:
git add filename
git rebase --continue

@bellstrand
Copy link
Contributor Author

@EisenbergEffect: I've rebased against on master now!
@AshleyGrant++;

@EisenbergEffect
Copy link
Contributor

@AshleyGrant Can you review this?

@AshleyGrant
Copy link
Collaborator

I'll pull it down and check it out. @bellstrand would you mind updating your commit message to match our commit message guidelines. Thanks!

@AshleyGrant
Copy link
Collaborator

Tests and code coverage ran fine.

@AshleyGrant
Copy link
Collaborator

Ashley@DESKTOP-2NT3EJC MINGW64 ~/github/skeleton-navigation/skeleton-esnext ((b77c0e9...))
$ gulp test
(node:5292) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
[14:38:48] Using gulpfile ~\github\skeleton-navigation\skeleton-esnext\gulpfile.js
[14:38:48] Starting 'test'...
08 07 2016 14:38:49.715:INFO [karma]: Karma v0.13.22 server started at http://localhost:9876/
08 07 2016 14:38:49.715:INFO [launcher]: Starting browser Chrome
08 07 2016 14:38:50.981:INFO [Chrome 51.0.2704 (Windows 10 0.0.0)]: Connected on socket /#SLZPGP5jbNdm-1L-AAAA with id 91262488
Chrome 51.0.2704 (Windows 10 0.0.0): Executed 11 of 11 SUCCESS (0.022 secs / 0.005 secs)
[14:38:51] Finished 'test' after 3 s

Ashley@DESKTOP-2NT3EJC MINGW64 ~/github/skeleton-navigation/skeleton-esnext ((b77c0e9...))
$ gulp cover
(node:5076) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
[14:39:43] Using gulpfile ~\github\skeleton-navigation\skeleton-esnext\gulpfile.js
[14:39:43] Starting 'cover'...
08 07 2016 14:39:45.187:INFO [karma]: Karma v0.13.22 server started at http://localhost:9876/
08 07 2016 14:39:45.187:INFO [launcher]: Starting browser Chrome
08 07 2016 14:39:46.468:INFO [Chrome 51.0.2704 (Windows 10 0.0.0)]: Connected on socket /#MQxd-b55f-btKlQRAAAA with id 35843361
------------------|----------|----------|----------|----------|----------------|
File              |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------|----------|----------|----------|----------|----------------|
 src\             |    92.86 |      100 |       75 |    92.86 |                |
  app.js          |      100 |      100 |      100 |      100 |                |
  child-router.js |      100 |      100 |      100 |      100 |                |
  users.js        |    85.71 |      100 |       75 |    85.71 |             12 |
------------------|----------|----------|----------|----------|----------------|
All files         |    92.86 |      100 |       75 |    92.86 |                |
------------------|----------|----------|----------|----------|----------------|

[14:39:46] Finished 'cover' after 3.04 s

@niieani
Copy link
Contributor

niieani commented Jul 8, 2016

This should be ported to the TypeScript JSPM skeleton before merging too.

@AshleyGrant
Copy link
Collaborator

The code coverage stuff uses a babel plugin. Would that even work over there?

@niieani
Copy link
Contributor

niieani commented Jul 8, 2016

Hmm. There's gotta be an alternative that also works for TypeScript in Gulp. I've got coverage working for both Babel and TypeScript in the Webpack skeletons, and it's done with the same libraries.

@AshleyGrant
Copy link
Collaborator

Maybe we could use that instead of this?

@niieani
Copy link
Contributor

niieani commented Jul 8, 2016

If we'd like both skeletons to be as close to each other as possible, we should probably use https://github.com/SBoudrias/gulp-istanbul together with remap-istanbul.
There's a tutorial here: https://www.sitepen.com/blog/2015/09/29/code-coverage-for-typescript-and-other-transpiled-languages/

@bellstrand
Copy link
Contributor Author

@AshleyGrant commit message updated to follow the guidelines.
@niieani What other lib have you found that generates a coverage report that shows the source, and not the transpiled code?
@AshleyGrant Open the generated html after running cover instead of just looking at the terminal output, theres where the big difference is.

@EisenbergEffect
Copy link
Contributor

@AshleyGrant @niieani Just checking to see where we are at on this.

@AshleyGrant
Copy link
Collaborator

I'm looking in to this currently

@EisenbergEffect
Copy link
Contributor

@AshleyGrant Where are we at with this currently?

@AshleyGrant
Copy link
Collaborator

I was never able to get anything working for TypeScript that would run the same way as this. The webpack solution (necessarily) runs against already transpiled code.

@niieani
Copy link
Contributor

niieani commented Jul 22, 2016

@AshleyGrant @EisenbergEffect The Webpack solution simply uses source-maps and remap-istanbul to regenerate coverage based on pre-transpiled code. This is possible to implement with gulp, here's how: https://github.com/SitePen/remap-istanbul#gulp-plugin

@plwalters
Copy link
Contributor

I think we should try to get this merged and create a new issue for the other skeletons if we can that way it doesn't become stale and need to rebase on anything.

@bellstrand
Copy link
Contributor Author

bellstrand commented Jul 27, 2016

@PWKad I disagree, if the way forward is with another library it's better to discard this.

Anyhow after this pull request was made babel-plugin-__coverage__ has been replaced with a "official" istanbul plugin babel-plugin-istanbul https://github.com/istanbuljs/babel-plugin-istanbul.
So if we decide that this is a good route, we should change to the new package and update the karma conf accordingly. Just give me a shout if you'd like me to fix that.

@niieani
Copy link
Contributor

niieani commented Jul 27, 2016

@bellstrand rebasing after 1.0.0 final and integrating the official plugin would be appreciated! Thanks. Seems like the JSPM TypeScript version will need a bit more love with the remap-istanbul post-processor.

@bellstrand bellstrand force-pushed the master branch 2 times, most recently from f03f8c5 to 1be9053 Compare July 28, 2016 13:41
Coverage report shows source instead of transpiled code

-
@bellstrand
Copy link
Contributor Author

@niieani I've rebased against master and swapped to the official plugin now.
Just give me a shout if there's anything more I should fix/do!

@niieani
Copy link
Contributor

niieani commented Jul 28, 2016

LGTM.
@EisenbergEffect could you ping somebody actively using JSPM that would want to give this a go?

@EisenbergEffect
Copy link
Contributor

@AshleyGrant You took a look at this earlier on. Would you be able to take a look at this update?

@niieani
Copy link
Contributor

niieani commented Aug 6, 2016

We should probably merge this before it goes stale. Can anybody confirm this works properly without breaking anything?

@EisenbergEffect
Copy link
Contributor

@AshleyGrant Can you review this one final time and advise? I'd like to merge this or close it in the next day.

@AshleyGrant
Copy link
Collaborator

Let's merge it. Just tested it. Works great.

@AshleyGrant
Copy link
Collaborator

@EisenbergEffect is @bellstrand good on CLA?

@niieani
Copy link
Contributor

niieani commented Sep 30, 2016

@AshleyGrant the new automated CLA checks shows that yes. See "details" in the check.

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

Successfully merging this pull request may close these issues.

5 participants