#7 fix PR#6 regression, add unit tests
Closed by tomh. Opened by dcallagh.
dcallagh/nodejs-packaging more-range-fixes  into  master

Download 7.patch

This fixes ^0.1 ranges which regressed in PR#6. Also fixes a few others that were previously broken, and adds a comprehensive set of assertions to cover all the various types of ranges.

Note that there are still some broken cases (commented out in the test). ~1.2.x is wrong currently but I spent nearly 2 hours messing around with the logic in convert_dep() and still couldn't figure out how to get it working without breaking other ranges.

FYI run the tests with pytest-3.

Just noticed that the hyphen ranges where the RHS is partial, are also wrong (that is, they are wrong in the code currently, and I accidentally copied the same error into the test case).

Amended the PR to reflect those broken cases.

rebased onto 45c1b7100f896df5aaec6b25670d70367231b57f

Wow, that's some hack...

Is it not easier to test it just by having a sample package.json and running the script on it and comparing the output? That's how I've always tested changes and it means you're testing the whole thing rather than breaking into the middle of it and testing one function...

Well, I wanted to try and cover exhaustively cover all the different kinds of versions ranges that NPM allows.

Just haphazardly testing it on a few package.json files does not seem like a good strategy since the likelihood of regressions is high.

See PR#8 for the same tests but with a rewrite of the version handling logic which passes all the cases.

There's no reason the coverage should be any less. I was just going to list the exact same test cases in a package.json file.

I'll try and have a look this evening anyway.

rebased onto 741bf9754acbd6b2da68cdc8e6e18f880b722d8d

Merged #8 instead.

Pull-Request has been closed by tomh

Metadata