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

Docs and more tests for #9087 #10312

Merged
merged 2 commits into from Jan 3, 2019
Merged

Docs and more tests for #9087 #10312

merged 2 commits into from Jan 3, 2019

Conversation

@papb
Copy link
Member

@papb papb commented Jan 3, 2019

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

The PR #9735 solved the issue #9087 very nicely. I was thinking that it deserved more tests, to help preventing regressions, and also deserved one section of the tutorial docs to be better explained.

This way, in this PR I have added a strong test to prevent regressions, and also some extra documentation.

@papb
Copy link
Member Author

@papb papb commented Jan 3, 2019

Note: for the tests I used a library called js-combinatorics, and for that I added it to package.json. According to packagephobia it is only 62 kB, so I think it should be fine. It is very useful to quickly assert that the order in which the merge is applied does not matter (which is something I wanted my test to check).

If adding extra dev-dependencies like this is not good, I can hard-code all the permutations directly, but that would be a bit ugly and unreadable (in my opinion).

@codecov
Copy link

@codecov codecov bot commented Jan 3, 2019

Codecov Report

Merging #10312 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10312   +/-   ##
=======================================
  Coverage   96.28%   96.28%           
=======================================
  Files          68       68           
  Lines        9772     9772           
=======================================
  Hits         9409     9409           
  Misses        363      363

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecb0868...352c708. Read the comment docs.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jan 3, 2019

Thanks @papb 👍

@sushantdhiman sushantdhiman merged commit 2a4e111 into sequelize:master Jan 3, 2019
4 checks passed
4 checks passed
codecov/patch Coverage not affected when comparing ecb0868...352c708
Details
codecov/project 96.28% remains the same compared to ecb0868
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@papb papb mentioned this pull request Jan 17, 2019
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants