add basic unit-tests for murmurhash3.js #7857
Conversation
There was a problem hiding this comment.
Nice work! This commit looks good to me with the comments addressed. Don't forget to squash multiple commits into one commit after addressing the comments (see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for how to do this easily if you're not familiar with the concept).
| @@ -0,0 +1,53 @@ | |||
| /* globals jasmine, expect, it, describe, MurmurHash3_64 */ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Nit: add a newline before this line.
| 'use strict'; | ||
|
|
||
| describe('MurmurHash3_64', function() { | ||
|
|
There was a problem hiding this comment.
Nit: remove this newline.
| hexdigest2 = hash.hexdigest(); | ||
| expect(hexdigest1).not.toEqual(hexdigest2); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Nit: remove this newline.
| describe('MurmurHash3_64', function() { | ||
|
|
||
| it('instantiates without seed ', function() { | ||
| var hash = new MurmurHash3_64(); |
There was a problem hiding this comment.
Indent this line with two spaces instead of four, i.e., remove two spaces. We always indent with two spaces. This also applies to the other occurrences in this file.
|
|
||
| describe('MurmurHash3_64', function() { | ||
|
|
||
| it('instantiates without seed ', function() { |
There was a problem hiding this comment.
Nit: remove the space at the end of the string. This also applies to the unit test below.
|
|
||
| var hexDigestExpected = 'f61cfdbfdae0f65e'; | ||
| var sourceText = 'test'; | ||
| var sourceCharCodes = [116,101,115,116]; // 't','e','s','t' |
There was a problem hiding this comment.
Nit: add spaces after each comma.
| var hexDigestExpected = 'f61cfdbfdae0f65e'; | ||
| var sourceText = 'test'; | ||
| var sourceCharCodes = [116,101,115,116]; // 't','e','s','t' | ||
| it('generates expected from string', function() { |
There was a problem hiding this comment.
I would change this to correctly generates a hash from a string (and Uint8Array/Uint32Array below) as it's a bit more descriptive in the logs when running the unit tests.
| expect(hash.hexdigest()).toEqual(hexDigestExpected); | ||
| }); | ||
|
|
||
| it('hexdigest changes after update without seed', function() { |
There was a problem hiding this comment.
I would change this to changes the hash after update without seed (and similar for the test below) to make it more readable in combination with the it function.
| it('hexdigest changes after update without seed', function() { | ||
| var hash = new MurmurHash3_64(); | ||
| var hexdigest1, hexdigest2; | ||
| hash.update('test'); |
There was a problem hiding this comment.
Since you defined var sourceText = 'test'; above, any reason not to just use hash.update(sourceText); here instead?
(This question applies in a couple of places below as well.)
jabiinfante
force-pushed
the
murmurhash3-unit-tests
branch
from
15b2c05
to
54fab60
Compare
|
@timvandermeij and @Snuffleupagus thanks for your comments. |
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/422569bf689d9d5/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/16c5f2515c45bb2/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/16c5f2515c45bb2/output.txt Total script time: 1.98 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/422569bf689d9d5/output.txt Total script time: 2.71 mins
|
|
Thank you for the patch! |
add basic unit-tests for murmurhash3.js
It adds some tests for murmurhash3 as suggested on #7261