chore: new store tests #1627
chore: new store tests #1627
Conversation
fbarbu15
added
the
E:End-to-end testing
size-limit report 📦
|
…re/new-store-tests
…re/new-store-tests
…re/new-store-tests
…re/new-store-tests
packages/tests/src/teardown.ts
Outdated
|
|
||
| import { NimGoNode } from "./index.js"; | ||
|
|
||
| const log = debug("waku:test"); | ||
|
|
||
| export function tearDownNodes( | ||
| export async function tearDownNodes( | ||
| nwakuNodes: NimGoNode[], |
There was a problem hiding this comment.
nit: I see we do tearDownNodes([nodeA], [nodeB]);
type can be changed to NimGoNode | NimGoNode[] and condition inside made like here
There was a problem hiding this comment.
Thanks, looks better with this change
packages/tests/src/utils.ts
Outdated
| @@ -0,0 +1,7 @@ | |||
| export function areUint8ArraysEqual(a: Uint8Array, b: Uint8Array): boolean { | |||
There was a problem hiding this comment.
nit: maybe some package can handle that for us as well as other utils we have specific for tests?
There was a problem hiding this comment.
Thanks for the suggestion, I found lodash for this specific function and started using it
Will review other "generic" custom test utils if they can be replaced with known packages but in another PR because this one is already big enough
packages/tests/src/utils.ts
Outdated
| @@ -0,0 +1,7 @@ | |||
| export function areUint8ArraysEqual(a: Uint8Array, b: Uint8Array): boolean { | |||
There was a problem hiding this comment.
Please use uint8arrays/equal as we already import this package and use it.
js-waku/packages/enr/src/enr.spec.ts
Line 8 in 0bc4a96
There was a problem hiding this comment.
Thanks, didn't knew about this one
| @@ -341,7 +342,7 @@ describe("Waku Filter V2: Subscribe", function () { | |||
| } | |||
|
|
|||
| // Check if both messages were received | |||
| expect(messageCollector.hasMessage(TestContentTopic, "M1")).to.be.true; | |||
| expect(messageCollector.hasMessage(newContentTopic, "M2")).to.be.true; | |||
| expect(messageCollector.hasMessage(TestContentTopic, "M1")).to.eq(true); | |||
There was a problem hiding this comment.
Oh, did we end having "true-ish" value returned instead of true?
There was a problem hiding this comment.
Yes, seems that .to.be.true was passing even when the value was not actually true. So i've replaced it with .to.eq(true) which seems stricter
There was a problem hiding this comment.
Yes, seems that
.to.be.truewas passing even when the value was not actuallytrue. So i've replaced it with.to.eq(true)which seems stricter
I'd try to fix your hasMessage function 🤔
There was a problem hiding this comment.
I've reviewed this function and similar functions that used .to.be.true and all the paths return True or False and they work as expected now. However those functions had changes in the recent path so it's indeed possible that they were missbehaving when I spotted this issue with .to.be.true.
| @@ -44,7 +44,7 @@ export class MessageCollector { | |||
| if (typeof message.payload === "string") { | |||
| return message.payload === text; | |||
| } else if (message.payload instanceof Uint8Array) { | |||
| return areUint8ArraysEqual(message.payload, utf8ToBytes(text)); | |||
There was a problem hiding this comment.
We have uint8arrays dependency already and it has uint8arrays/equal
packages/tests/src/teardown.ts
Outdated
| @@ -26,7 +29,7 @@ export async function tearDownNodes( | |||
| } | |||
| }); | |||
|
|
|||
| const stopWakuNodes = wakuNodes.map(async (waku) => { | |||
There was a problem hiding this comment.
I recently thought about this and remoteNode might be the best descriptive
There was a problem hiding this comment.
This code is outdated and not sure I understand you suggestion.
There was a problem hiding this comment.
I think I saw a wNode name and we have a weird "NimGoWaku" naming var in the code. I am saying that maybe we should use "remoteNode" as a terminology instead. It's a nitpick. Cc @waku-org/js-waku-developers
There was a problem hiding this comment.
oh, ok that makes sense. Will update it with a next PR
Problem
There were only 9 store tests and the protocol was lacking in coverage
Solution
Reached ~60 tests and increased coverage
Notes
In this PR I also did some refactoring to common methods so they work for store as well. In doing this there are some updates to other tests from other protocols.
Also I increased some timeouts to some flaky tests/methods