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

Test Refactor #136

Merged
merged 12 commits into from
Mar 10, 2019
Merged

Test Refactor #136

merged 12 commits into from
Mar 10, 2019

Conversation

MarkHerhold
Copy link
Contributor

@MarkHerhold MarkHerhold commented Mar 5, 2019

The existing tests are a mess right now and need to be cleaned up.

  • fix innefective assertions / swap should.js to chai
  • linting
  • code coverage
  • split tests up
  • update test dependencies

I introduced eslint with the airbnb style (it's reasonably similar) for linting and c8 for coverage. In order to keep this PR small and focused, we are not linting/changing the koa-body codebase.

I'm also dropping Node.js 6 from the CI test matrix because it is EOL as of April 2019, which is about when I think we can roll out the next major version of koa-body.

@MarkHerhold MarkHerhold marked this pull request as ready for review March 7, 2019 02:53
@MarkHerhold
Copy link
Contributor Author

Pinging @damianb @thuitaw and @zevisert for feedback.

@katanacrimson
Copy link
Contributor

katanacrimson commented Mar 7, 2019

We will need to be sure we're very clear about (effectively) dropping node 6 support here. While yes, it is EOL soon, we are removing it from the tests, which is equivalent to saying we're not checking to see if things don't work on anything under node 8.

Overall though, looks good.

ED: Reread - noticing now that you're implying this will be something for a major release. I like this idea; we should also be considering another issue/pr for introducing full linting of the codebase, and one more for removing the compat shim. I'll open the latter in a few.

Copy link
Contributor

@zevisert zevisert left a comment

Choose a reason for hiding this comment

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

Awesome to see these tests being cleaned up, a few general comments:

  1. It would be good to use this time to also bring in something like husky to run tests automatically as a git hook
  2. There's still various assertion schemes across the different tests
  3. I don't know how you want to enforce my review, so I'm not giving explicitly approving or denying

test/README.md Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/unparsed.js Outdated Show resolved Hide resolved
@MarkHerhold MarkHerhold mentioned this pull request Mar 8, 2019
11 tasks
@MarkHerhold
Copy link
Contributor Author

@zevisert I switched out should.js for chai. I like your idea to use husky and if you want to PR that, I'd be happy to review.

Thank you both for the review comments.

@MarkHerhold MarkHerhold merged commit 3bec89f into master Mar 10, 2019
@MarkHerhold MarkHerhold deleted the refactor-tests branch March 10, 2019 12:16
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