-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
chore: new lightpush tests #1571
Conversation
size-limit report 📦
|
…re/new-lightpush-tests
@@ -0,0 +1,193 @@ | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can rename the file simply utils.ts
since it's in tests/light-push
already
} | ||
} | ||
|
||
export async function runNodes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably generalise this as a util itself for a scope of all tests
} | ||
} | ||
|
||
export function tearDownNodes(nwaku: NimGoNode, waku: LightNode): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generalise this for the entire tests scope as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to src
@@ -0,0 +1,227 @@ | |||
import { createEncoder, DefaultPubSubTopic } from "@waku/core"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can rename file to index.spec.ts
return this.list[index]; | ||
} | ||
|
||
async waitForMessages( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can probably be generalised for the scope of all tests as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added all message collection logic to src/message_collector.ts and made it work for both MessageRpcResponse and DecodedMessage
Thanks for the hint
…re/new-lightpush-tests
} | ||
} | ||
|
||
export async function runNodes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this configurable to allow arbitrary number of waku
and nwaku
to be started with this?
something like:
runNodes(content, {nwaku: 3, jswaku: 1})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but we would need to add lots of parameters to runNodes to make it trully generic
And looking at other tests/protocols, most of the have different setup needs
export const messageText = "Light Push works!"; | ||
export const messagePayload = { payload: utf8ToBytes(messageText) }; | ||
|
||
export async function runNodes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, all protocol tests would have their own version of runNodes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what I saw, it's just filter and lightpush. The other protocols would not use similar setup-node code. That's why I didn't put it in a global util functions like the others.
And event between filter and lightpush the code is pretty different: different nwaku args, different createLightNode args, different waitForRemotePeer protocols.
I can merge them into one but not sure how much value that will add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Thank you! Can you please merge it? I don't have access |
Problem
Similar to #1552
Solution
Added PR with following fixes: