bower / decompress-zip

Module that decompresses zip files
MIT License
102 stars 76 forks source link

Tests refactoring #37

Closed szwacz closed 9 years ago

szwacz commented 9 years ago

As conversation in https://github.com/bower/decompress-zip/pull/32 went, here are refactored unit tests.

Main improvements:

  1. Zip files are stored inside this repo
  2. There are as small amount of test files as possible
  3. Tests are runnable on Windows

The idea: Next to test package we are also storing recipie how to build this zip package, and explanation what it tests. In current tests there is no info what which zip file actually tries to test (what is confusing as hell). Every test pack contains also json file which specs content inside the zip file, so we have one object to test uncompressed files against. For testing I'm using my own library because it is perfect fit for this job. The lib is quite young still, but I give lifetime guarantee ;).

Code coverage: There were many files used with previous tests, but all of them were collected in manner "just put together couple of common archives and call it a day", so to my surprise I achieved higher code coverage with only two, better designed zip files.

Coverage of old tests:

Statements   : 77.13% ( 253/328 )
Branches     : 65.52% ( 57/87 )
Functions    : 67.19% ( 43/64 )
Lines        : 77.13% ( 253/328 )

Coverage after refactoring:

Statements   : 78.77% ( 256/325 )
Branches     : 57.47% ( 50/87 )
Functions    : 71.88% ( 46/64 )
Lines        : 78.77% ( 256/325 )

So I stopped with only those two zips. If you want to raise code coverage higher we need to know what we actually have to test. Just throwing next zips on the pile as previous tests shows is not very effective.

sheerun commented 9 years ago

Thank you for this PR. I'll try to review soon.

My only concern is about 2 zip files for 78% test coverage. I think each zip file should represent special case that can fail instead containing 10 use cases. The same way unit tests test only few lines of code.

sheerun commented 9 years ago

Also, please make travis build pass

szwacz commented 9 years ago

The Travis CI is failing on:

grunt test-files
Running "exec:test-files" (exec) task
Verifying property exec.test-files exists in config...ERROR
>> Unable to process task.
Warning: Required config property "exec.test-files" missing. Use --force to continue.

It tries to download old test assets from remote server. This is no longer needed step fot this PR, and this code has been removed from the source.

sheerun commented 9 years ago

Can you remove it from .travis.yml and Gruntfile?

szwacz commented 9 years ago

Oh, that's where it sits. Done.

sheerun commented 9 years ago

Looks OK for me.

prust commented 9 years ago

@sheerun: FYI, the travis failure on e8a6875 is an intermittent issue -- similar to the one encountered here: https://github.com/bower/decompress-zip/pull/26#issuecomment-43798022. It boils down to a dependency introducing a ^ semver expression as part of a patch update. Since decompress-zip (and most node libraries) automatically pull in patch updates, the change gets pulled in and starts breaking Travis's 0.8 build, which uses a very old version of npm that doesn't support ^.

There is long-standing tension between npm and node about whether node should ship a newer version of npm with node 0.8 (see https://github.com/joyent/node/pull/7797) -- one that supports ^ expressions in semver strings. One way out of this intermittent breakage on 0.8 is to update npm in the before_install section of travis.yml:

before_install:
- '[ "${TRAVIS_NODE_VERSION}" != "0.8" ] || npm install -g npm@1.4.28'
sindresorhus commented 9 years ago

@szwacz Happy to land this if you can fix the merge conflict.

sheerun commented 9 years ago

Fixed and merged to master. Thanks!