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

add test coverage #244

Closed
bcoe opened this issue Nov 11, 2016 · 12 comments
Closed

add test coverage #244

bcoe opened this issue Nov 11, 2016 · 12 comments
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@bcoe
Copy link
Contributor

bcoe commented Nov 11, 2016

I would love to help add test coverage to this repo, using nyc and potentially babel-plugin-istanbul. Would you accept a pull request for this?

@rauchg
Copy link
Member

rauchg commented Nov 12, 2016

Sounds great. And a badge!

@arunoda
Copy link
Contributor

arunoda commented Nov 12, 2016

And I'd like to see some test cases as well.
@rauchg do you have any plans on how to do testing?
Currently, it's just a few integration test cases.

How about some unit tests for specially stuff in the lib directory?

@bcoe
Copy link
Contributor Author

bcoe commented Nov 13, 2016

@rauchg would you be open to using babel-register for tests, rather than a compile step; or would you prefer to wire up source-maps?

I have a slight preference for babel-register, but can take on either approach (source-maps being slightly more of a hassle).

@rauchg
Copy link
Member

rauchg commented Nov 13, 2016

@bcoe I tend to use babel-register for tests as well

@nkzawa
Copy link
Contributor

nkzawa commented Nov 14, 2016

@bcoe @rauchg we don't use babel-register for now since need to compile the source and run tests at the same time in some cases anyway.

@nkzawa nkzawa added enhancement good first issue Easy to fix issues, good for newcomers labels Nov 14, 2016
@bcoe
Copy link
Contributor Author

bcoe commented Nov 15, 2016

two options for you here:

screen shot 2016-11-14 at 4 07 25 pm

☝️ implement just the files currently touched by tests -- a less terrifying view of the world.

screen shot 2016-11-14 at 4 09 07 pm

Instrument everything, a more honest view of the world.

Next question, are there any folders you see there that should be excluded from coverage? CC: @nkzawa, @rauchg

@rauchg
Copy link
Member

rauchg commented Nov 15, 2016

omg this is so beautiful I want to cry 😭

@bcoe
Copy link
Contributor Author

bcoe commented Nov 15, 2016

@rauchg @nkzawa see #259, I did my best to stay in the spirit of the project's current structure.

@sedubois
Copy link
Contributor

sedubois commented Nov 19, 2016

Opened #278 to raise the observation that, besides performance, Jest has built-in test coverage.

@bcoe
Copy link
Contributor Author

bcoe commented Nov 19, 2016

@sedubois Jest uses istanbul-lib-coverage and babel-plugin-istanbul behind the scenes to facilitate its coverage, both libraries that I maintain and are part of #259 . I personally like having nyc in the tool-chain:

  • it provides a simple CLI interface for outputting tests in various formats:
    • HTML which helps you dig into where your coverage is lacking.
    • lcov which allows you to pipe it to coveralls, which @rauchg had requested for badges.
  • nyc also supports sub-processes, which is useful for instrumenting everything in the next.js/bin.

any ways, I would happily help write some more tests, but my personal preference would be to stick with AVA right now and to get some more tests written; rather than swapping technologies.

@sindresorhus can you speak the the advantages/disadvantages of using AVA vs. Jest for testing a react application, I was reading this document on the technical considerations:

https://github.com/avajs/ava/blob/master/docs/recipes/react.md

@nkzawa
Copy link
Contributor

nkzawa commented Nov 19, 2016

This tweet makes sense https://twitter.com/sindresorhus/status/774244618057555970
We are trying to introduce babel-core/register but it will gradually make our tests slow.
I think Jest can be a better choice for next.js.

@arunoda
Copy link
Contributor

arunoda commented Nov 19, 2016

I also like to move into Jest. Since we don't have many tests, it's better to decide this soon and work on actually writing tests.

@nkzawa nkzawa closed this as completed Dec 2, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants