Add tests for Number constructor that highlight deficiencies in certain environments. #366
Conversation
|
There are a couple of others that I will try to add, but you should have everything you need. |
| @@ -387,5 +387,31 @@ describe('Number', function () { | |||
| expect(Number('0o11')).to.equal(9); | |||
| expect(Number({ toString: function () { return '0o12'; }, valueOf: function () { return '0o13'; } })).to.equal(11); | |||
| }); | |||
|
|
|||
| it('should produce NaN', function () { | |||
There was a problem hiding this comment.
these tests are failing but it's very hard to figure out which line is failing.
Perhaps expect(String(Number(…))).to.equal('NaN') would be better? I don't think we need to worry about testing that Number() returns a number :-)
|
Changed those to String and 'NaN', hope that helps you a little more. On Node v0.10 and v0.12 the very first test on line 392 is failing to begin with. |
|
Thanks, this is highlighting a real failure. Can you please rebase these two commits down to one? I'll merge it manually, along with the fix. |
…in environments. Altered tests as suggested
|
There are some other tests that highlight other issues but I guess we can take those separately? |
|
I can leave this PR open, and you can keep adding commits to it, but if you rebase the existing diff into a single commit, I can cherry-pick it into master. |
|
I can do that when I have a little time (next week), I think some of this affects your https://github.com/ljharb/es-abstract and https://github.com/ljharb/es-to-primitive projects. |
|
It definitely does - please open issues/PRs there as well when you can :-) |
|
Turns out |
|
No problem, I think you still have the badHex issue though, some older environments, node v0.8 accepts +/-Hex value. But you may not be worried about such environments. I think the other (missing tests) are to check whitespace trimming, making sure that the correct ones are stripped. |
|
@Xotic750 I am indeed worried about those environments - more tests are welcome. |
These were recently fixed in lodash trunk, _.toNumber.
Original PR lodash/lodash#1577
Actual merge lodash/lodash@3795534