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

added unit tests for package-hoister #2736

Merged
merged 2 commits into from
Feb 27, 2017

Conversation

rally25rs
Copy link
Contributor

Summary

Add a series of unit tests for PackageHoister.

Previously there was only 1 unit test (not including integration tests).

With this PR, there are now enough unit testa to bump coverage of the package-hoister.js file to ~93% (correction, after I rebased from current master it dropped to ~88%. I guess more code was added recently). I really wanted to get as close to 100% coverage as possible, but I've been starting at this one file for a couple days and not sure what scenarios would hit some branches in the code.

The real motivation was to understand how PackageHoister works in hopes of trying to look more into issue #2673 and I figured, how better to figure out how it works than to write tests for it!

Test plan

$ npm run test-only __tests__/package-hoister.js

----------------------|----------|----------|----------|----------|----------------|
File                  |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------------------|----------|----------|----------|----------|----------------|
All files             |    53.86 |    45.89 |    44.87 |    54.45 |                |
 src                  |    59.31 |    53.25 |    54.55 |    58.75 |                |
  package-hoister.js  |    88.75 |    72.16 |      100 |    88.41 |... 494,497,498 |
  package-resolver.js |    17.26 |    21.05 |    24.24 |    17.37 |... 453,459,461 |
 src/util             |    30.93 |    24.53 |    21.74 |    30.56 |                |
  blocking-queue.js   |    21.05 |    18.75 |    21.43 |    21.05 |... 133,134,135 |
  map.js              |    72.73 |    55.56 |      100 |       70 |         7,8,16 |
  misc.js             |    34.48 |    16.67 |     12.5 |       60 |          60,62 |
----------------------|----------|----------|----------|----------|----------------|
Test Suites: 1 passed, 1 total
Tests:       10 passed, 10 total
Snapshots:   0 total
Time:        1.258s
Ran all test suites matching "__tests__/package-hoister.js".

Additional notes

if you run all the tests, there seems to be 2 Flow warnings and 3 test errors. These happened after I merged in the current master code and I believe they are errors in current master, not my added test code.

I made a custom jasmine matcher in the test file to clean up the tests. Jest has its own way of doing that, but Yarn seems to be on an older version of Jest, so I had to add it right to Jasmine. Flow was not happy with this new matcher, so I added it to the main type definition for Jasmine in yarn/flow-typed/npm/jest-v14.0.x.js even though that function only exists in __tests__/package-hoister.js but I had no idea how to tell Flow to allow that method only in that one file. If someone has a better idea, please let me know!

@bestander
Copy link
Member

Love it!
Thanks a lot, this will make it way better going forward

@bestander bestander merged commit 437ca02 into yarnpkg:master Feb 27, 2017
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.

2 participants