toArray/toString: add tests

[dcousens]

This PR adds tests for toArray and toString to enforce consistency. The tests as of right now are failing because I am not aware of the intended output format of toArray vs toString.

toString() seems to output decimal. Tests pass. toString(16) seems to output unsigned hex string with a - sign appended to the front. toArray() seems to output unsigned binary, but not in twos complement (the standard)

@indutny I'm happy to amend these tests if you could shed some light on what is expected :).

[dcousens]

The returned hex from toString also seems to be incomplete, in that hex.length % 2 === 0 doesn't always hold true. In fact, 0x01 will be returned as 0x1. Is it intentional that the hex string is not grouped by bytes?

[dcousens]

ping @indutny

[indutny]

I don't think that we need invalid section, at least not now.

[indutny]

Otherwise LGTM, thank you! ;)

[dcousens]

Rebased.

Ok @indutny I addressed most of the nits.

Waiting on your answer about whether it should have a the default zero padding of 1 (which IMHO I think is odd).

Also curious if after the discussion about regex's whether I keep/remove the invalid section.

[indutny]

@dcousens looking very good now, except positive-only hex and a failing tests... Speaking of this, I'm not a big fan of having failing tests in repo's master. Would you mind separating them into an another PR, where we'll figure out the best way to make it work properly?

[dcousens]

Sure, did you have a branch I could target? On 11 Apr 2015 04:29, "Fedor Indutny" [email protected] wrote:

@dcousens https://github.com/dcousens looking very good now, except positive-only hex and a failing tests... Speaking of this, I'm not a big fan of having failing tests in repo's master. Would you mind separating them into an another PR, where we'll figure out the best way to make it work properly?

— Reply to this email directly or view it on GitHub https://github.com/indutny/bn.js/pull/36#issuecomment-91645821.

[indutny]

@dcousens oh right! Added you as a collaborator to this repo, I hope you don't mind :)

[axic]

@dcousens do you think it would make sense revisiting this PR?

[jprichardson]

@axic I sure hope so.

Garbage sounds like a real easy way to have people encounter issues without knowing it. Especially given this library is most often used for crypto. I think it is likely people will just accept the garbage result as what it should be at face value, because the underlying implementation they have no understanding of.

I couldn't have said it any better @dcousens!

[axic]

Some of the things you've mentioned could be addressed/discussed as part of #129:

toString(16) seems to output unsigned hex string with a - sign appended to the front. toArray() seems to output unsigned binary, but not in twos complement (the standard)

[dcousens]

Added this back into my work queue :+1:

edit 29/09/2016: soon...

edit 30/11/2017: :disappointed:

[dcousens]

re-adding to work queue attempt 3

[earonesty]

Note: right now the toString() function doesn't zero pad hex (or anything > base 10), which results in strings like "a" instead of "0a" being output as valid hex strings. Perhaps this is causing tests to fail. If people are relying on the current behavior, might need to make "padding" an option.