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.

Sign up for GitHub

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

Jump to bottom

toArray/toString: add tests #36

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

toArray/toString: add tests #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
11d82d0
tests: remove unused fixtures variables and move fixtures to fixtures/
dcousens Feb 12, 2015
d6d252c
tests: remove unused variables
dcousens Feb 12, 2015
8b13595
tests: add tests/fixtures for toArray/toString
dcousens Feb 12, 2015
7d9198b
rename dhGroups to dh-groups
dcousens Apr 9, 2015
1b56590
utils-test: adhere to 80char limit
dcousens Apr 9, 2015
6e065cb
utils-test: test initialization with hex
dcousens Apr 9, 2015
dc79116
utils-test: more 80char limit adherence
dcousens Apr 9, 2015
59a2122
fixtures: dec of 0 should be hex of 00
dcousens Apr 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/arithmetic-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
var assert = require('assert');
var BN = require('../').BN;
var fixtures = require('./fixtures');
var fixtures = require('./fixtures/dh-groups');

describe('BN.js/Arithmetic', function() {
describe('.add()', function() {
Expand Down Expand Up @@ -183,8 +183,8 @@ describe('BN.js/Arithmetic', function() {
});

it('should regress mul big numbers', function() {
var q = fixtures.dhGroups.p17.q;
var qs = fixtures.dhGroups.p17.qs;
var q = fixtures.p17.q;
var qs = fixtures.p17.qs;

var q = new BN(q, 16);
assert.equal(q.sqr().toString(16), qs);
Expand Down
1 change: 0 additions & 1 deletion test/binary-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
var assert = require('assert');
var BN = require('../').BN;
var fixtures = require('./fixtures');

describe('BN.js/Binary', function() {
describe('.shl()', function() {
Expand Down
1 change: 0 additions & 1 deletion test/constructor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
var assert = require('assert');
var BN = require('../').BN;
var fixtures = require('./fixtures');

describe('BN.js/Constructor', function() {
describe('with Smi input', function() {
Expand Down
105 changes: 105 additions & 0 deletions test/fixtures/bn.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
{
"valid": [
{
"dec": "0",
"hex": "00"
},
{
"dec": "1",
"hex": "01"
},
{
"dec": "-1",
"hex": "ff"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this happen? Hex could be negative too, right?

Copy link
Contributor Author

@dcousens dcousens Apr 10, 2015 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the twos complement, that is negative.
On 11 Apr 2015 04:28, "Fedor Indutny" [email protected] wrote:

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm...? I don't think I support twos complement anywhere in code. Also, it doesn't seem to be a desired feature to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcousens HEX is nothing special to .toString(), the base-36 or base-3 encoding should work the same way as the HEX does.

I guess the two's complement thing should happen in the code responsible for encoding this. There are various way to encode it, and this doesn't look like a reason to make HEX something special.

Copy link
Contributor Author

@dcousens dcousens Apr 10, 2015 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, maybe it's worth having it as separate functionality? In any
case, I'll amend the test vectors.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense for another issue/PR ;)

},
{
"dec": "127",
"hex": "7f"
},
{
"dec": "-127",
"hex": "81"
},
{
"dec": "128",
"hex": "80"
},
{
"dec": "-128",
"hex": "80"
},
{
"dec": "255",
"hex": "ff"
},
{
"dec": "-255",
"hex": "ff01"
},
{
"dec": "16300",
"hex": "3fac"
},
{
"dec": "-16300",
"hex": "c054"
},
{
"dec": "62300",
"hex": "f35c"
},
{
"dec": "-62300",
"hex": "ff0ca4"
},
{
"dec": "158798437896437949616241483468158498679",
"hex": "77777777777777777777777777777777"
},
{
"dec": "115792089237316195423570985008687907852837564279074904382605163141518161494336",
"hex": "fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140"
},
{
"dec": "48968302285117906840285529799176770990048954789747953886390402978935544927851",
"hex": "6c4313b03f2e7324d75e642f0ab81b734b724e13fec930f309e222470236d66b"
}
],
"invalid": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, none of these are throwing. Is that intentional?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is for speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean, for speed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that checking and throwing takes time, and I want it to be as fast as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, what would the expected behaviour be then? Just so I can put it under these tests :)

Like, does it just skip bad characters?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worse than that :) You'll get some garbage. Predictable, but still a garbage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make the default safe, and for the internal usage a factory method BN.__createFast which doesn't do the checks?
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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcousens I don't even use .toString()/parse internally, actually. I wonder if the regexp in assert before entering the _parseHex and _parseBase would be enough? Though, it might be a bit more complicated for _parseBase, since the character set may different between different bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would most certainly be a start.
I'd have to look at the implementations for other ways to maintain performance and increase safety.

{
"description": "non-decimal string",
"dec": "invalid"
},
{
"description": "non-hex string",
"hex": "invalid"
},
{
"description": "incomplete hex",
"hex": "ffffd0a"
},
{
"description": "non-decimal alphabet",
"dec": "c2F0b3NoaQo="
},
{
"description": "non-hex alphabet",
"hex": "c2F0b3NoaQo="
},
{
"description": "internal whitespace",
"dec": "11111 11111",
"hex": "11111 11111"
},
{
"description": "leading whitespace",
"dec": " 1111111111",
"hex": " 1111111111"
},
{
"description": "trailing whitespace",
"dec": "1111111111 ",
"hex": "1111111111 "
}
]
}
2 changes: 1 addition & 1 deletion test/fixtures.js → test/fixtures/dh-groups.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
exports.dhGroups = {
module.exports = {
p16: {
prime: 'ffffffffffffffffc90fdaa22168c234c4c6628b80dc1cd1' +
'29024e088a67cc74020bbea63b139b22514a08798e3404dd' +
Expand Down
3 changes: 1 addition & 2 deletions test/pummel/dh-group-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
var assert = require('assert');
var BN = require('../../').BN;
var fixtures = require('../fixtures');
var groups = require('../fixtures/dh-groups');

describe('BN.js/Slow DH test', function() {
var groups = fixtures.dhGroups;
Object.keys(groups).forEach(function(name) {
it('should match public key for ' + name + ' group', function() {
var group = groups[name];
Expand Down
1 change: 0 additions & 1 deletion test/red-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
var assert = require('assert');
var BN = require('../').BN;
var fixtures = require('./fixtures');

describe('BN.js/Reduction context', function() {
function testMethod(name, fn) {
Expand Down
63 changes: 57 additions & 6 deletions test/utils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,97 @@
var assert = require('assert');
var BN = require('../').BN;
var fixtures = require('./fixtures');
var fixtures = require('./fixtures/bn');

describe('BN.js/Utils', function() {
describe('.toString()', function() {
fixtures.valid.forEach(function(f) {
it('should return ' + f.dec + ' when built with ' + f.dec, function() {
var bi = new BN(f.dec);

assert.equal(bi.toString(), f.dec);
});

it('should return ' + f.dec + ' when built with ' + f.hex, function() {
var bi = new BN(f.hex, 16);

assert.equal(bi.toString(), f.dec);
});

it('should return big-endian twos complement' +
'hex for ' + f.dec, function() {
var bi = new BN(f.dec);

assert.equal(bi.toString('hex'), f.hex);
});
});

describe('hex padding', function() {
it('should have length of 8 from leading 15', function() {
var a = new BN('ffb9602', 16);
var b = new Buffer(a.toString('hex', 2), 'hex');

assert.equal(a.toString('hex', 2).length, 8);
});

it('should have length of 8 from leading zero', function() {
var a = new BN('fb9604', 16);
var b = new Buffer(a.toString('hex', 8), 'hex');

assert.equal(a.toString('hex', 8).length, 8);
});

it('should have length of 8 from leading zeros', function() {
var a = new BN(0);
var b = new Buffer(a.toString('hex', 8), 'hex');

assert.equal(a.toString('hex', 8).length, 8);
});

it('should have length of 64 from leading 15', function() {
var a = new BN(
'ffb96ff654e61130ba8422f0debca77a0ea74ae5ea8bca9b54ab64aabf01003',
16);
var b = new Buffer(a.toString('hex', 2), 'hex');

assert.equal(a.toString('hex', 2).length, 64);
});

it('should have length of 64 from leading zero', function() {
var a = new BN(
'fb96ff654e61130ba8422f0debca77a0ea74ae5ea8bca9b54ab64aabf01003',
16);
var b = new Buffer(a.toString('hex', 64), 'hex');

assert.equal(a.toString('hex', 64).length, 64);
});
});
});

describe('.toArray()', function() {
fixtures.valid.forEach(function(f) {
it('should return a big-endian ' +
'twos complement integer for ' + f.dec, function() {
var bi = new BN(f.dec);
var ba = new Buffer(bi.toArray());

assert.equal(ba.toString('hex'), f.hex);
});
});

fixtures.invalid.forEach(function(f) {
if (f.dec) {
it('should throw on ' + f.dec, function() {
assert.throws(function() {
new BN(f.dec);
});
});
}

if (f.hex) {
it('should throw on ' + f.hex, function() {
assert.throws(function() {
new BN(f.hex, 16);
});
});
}
});
});

describe('.bitLength()', function() {
it('should return proper bitLength', function() {
assert.equal(new BN(0).bitLength(), 0);
Expand Down