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

Consider adding code coverage #335

Merged
merged 9 commits into from
Jan 26, 2015
Merged

Consider adding code coverage #335

merged 9 commits into from
Jan 26, 2015

Conversation

pascalduez
Copy link
Member

Just trying out.

Provides some interesting insights on how the codebase is tested.
Travis reports directly to https://coveralls.io
Coveralls add a comment on PRs to report coverage changes. See SassDoc/sass-convert#8

What do you think, useful or not ?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2fd6d28 on coverage into * on develop*.

@pascalduez
Copy link
Member Author

screen shot 2015-01-25 at 18 18 28

@KittyGiraudel
Copy link
Member

Okay on core repository only.

@valeriangalliat
Copy link
Member

We maybe need to refactore CLI tests in makefile to be executed through Node so we can test CLI coverage?

@pascalduez
Copy link
Member Author

We maybe need to refactore CLI tests in makefile to be executed through Node so we can test CLI coverage?

That's what I thought, but does not seem to really influence the result ?

There is a minor issue:
Coveralls fetch files from Github to display them, but dist is not git tracked.
https://coveralls.io/r/SassDoc/sassdoc
Actually we don't really need coveralls, except if we want a nice badge and make the results public.
Istanbul provides html reports: make view-cover.

@valeriangalliat
Copy link
Member

view-cover should depend on cover (empty result if make cover was not manually executed before).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b8073e8 on coverage into * on develop*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ac24600 on coverage into * on develop*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9dcb25e on coverage into * on develop*.

@KittyGiraudel
Copy link
Member

When is this going to be merged?

@pascalduez
Copy link
Member Author

Depends whether we get a consensus from @SassDoc/owners ?

I personally like it, it's a good way to motivates us to write more tests, and also discover hidden issues.
The coveralls integration is not mandatory.

@valeriangalliat
Copy link
Member

👍

@FWeinb
Copy link
Member

FWeinb commented Jan 26, 2015

👍

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling dd76770 on coverage into * on develop*.

pascalduez added a commit that referenced this pull request Jan 26, 2015
@pascalduez pascalduez merged commit 7bbf6e7 into develop Jan 26, 2015
@pascalduez pascalduez deleted the coverage branch January 26, 2015 19:56
@pascalduez
Copy link
Member Author

Most recent status.
Would be interesting to add tests for Environment.

screen shot 2015-01-26 at 20 57 04

@KittyGiraudel
Copy link
Member

Shall we open an issue about increasing test coverage? 80% is pretty neat, but I'd like us to bump 90-95% if it's doable.

@pascalduez
Copy link
Member Author

We won't reach 100% anyway, due to the ES(6|2015) transpilation, there's some snippets from 6to5 that aren't called, and would make no sense for us to test them.
But for sure, we can improve things.

@pascalduez pascalduez mentioned this pull request Jan 27, 2015
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants