API Tests for the bloom module #330
Conversation
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
| const b = new Bloom() | ||
| st.deepEqual(b.bitvector, utils.zeros(byteSize), 'should be empty') | ||
| st.end() | ||
| }) |
| st.throws(() => new Bloom('invalid'), /AssertionError/, 'should fail for invalid type') | ||
| st.throws(() => new Bloom(utils.zeros(byteSize / 2), /AssertionError/), 'should fail for invalid length') | ||
| st.end() | ||
| }) |
tests/api/bloom.js
Outdated
| st.true(b.check('value 1'), 'should contain value 1') | ||
| st.true(b.check('value 2'), 'should contain value 2') | ||
| st.end() | ||
| }) |
There was a problem hiding this comment.
I actually am no bloom filter expert myself, could you explain - just for my understanding - where this 'value ' part of 'value 1' can be re-found in the hex string?
There was a problem hiding this comment.
Back then I also looked at a helpful tutorial to learn their details, which mentions "To add an element to the Bloom filter, we simply hash it a few times and set the bits in the bit vector at the index of those hashes to 1."
tests/api/bloom.js
Outdated
| const b = new Bloom() | ||
| st.false(b.check('random value'), 'should not contain random value') | ||
| st.end() | ||
| }) |
There was a problem hiding this comment.
This is already high-level criticism, don't necessarily needs to be updated, but the explanatory strings are bit misleading on first read, would be more clear with something like 'should not contain string "random value"', a bit similar for other test messages.
Again, change or keep as you like - just as a note and maybe a bit picky. 😄
Otherwise: OK.
| let found = b.check('value') | ||
| st.true(found, 'should contain added value') | ||
| st.end() | ||
| }) |
| let found = b.multiCheck(['value 1', 'value 2']) | ||
| st.true(found, 'should contain both values') | ||
| st.end() | ||
| }) |
There was a problem hiding this comment.
Phew, the whole usage of types (string vs Buffer) in the Bloom implementation respectively the corresponding documentation is really a mess: but right, this is exactly what the tests you have written are for to discover! 😄 I've opened an according issue #332.
Within the current implementation the Buffer conversion in the Bloom.multiCheck() method has to be removed, opened the following issue on this: #333
Re-architecturing whole parts of the library definitely doesn't have to be part of the scope of the work you are doing here, it's already great that we've discovered this now.
If you want you can just comment out this test, then we can re-add once the issue from above is fixed. I would then approve this part of your PR series.
tests/api/bloom.js
Outdated
| b1.or(b2) | ||
| st.true(b1.check('value 2'), 'should contain value 2 after or') | ||
| st.end() | ||
| }) |
tests/api/bloom.js
Outdated
| st.end() | ||
| }) | ||
|
|
||
| t.test('should generate the correct bloom filter value ', (st) => { |
There was a problem hiding this comment.
Can you remove the blank space after 'value ' on this occasion?
| bloom.bitvector.toString('hex'), | ||
| '00000000000000000000080000800000000000000000000000000000000000000000000000000000000000000000000000000000000004000000080000200000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000010008000000000000000002000000000000000000000000008000000000000' | ||
| ) | ||
| st.end() |
|
TBH I've no idea what's wrong with this |
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
This is the first sub-PR of #315, and contains only the tests added for
bloom. I've cherry-picked commits from the other branch. Please let me know if you'd rather have it done some other way.One case which is meant to test bloom's multiCheck fails.
multiCheckconverts inputs via hex to buffer, althoughcheckitself doesn't do such a thing.This PR also fixes #333