test(sharding): Implement sharding tests #2603
Conversation
AlejandroCabeza
changed the title
|
You can find the image built from this PR at Built from 1c9e81b |
AlejandroCabeza
force-pushed
the
tests/sharding
branch
from
026ef2a to
82978c5
Compare
|
You can find the image built from this PR at Built from 62d1c80 |
|
You can find the image built from this PR at Built from 62d1c80 |
| serverHandler = server.subscribeCompletionHandler(topic) | ||
| clientHandler = client.subscribeCompletionHandler(topic) | ||
|
|
||
| # await sleepAsync(FUTURE_TIMEOUT) |
There was a problem hiding this comment.
Let's better remove that comment :)
| discard await server.publish( | ||
| some(topic), | ||
| WakuMessage(payload: "message2".toBytes(), contentTopic: "contentTopic"), | ||
| ) |
There was a problem hiding this comment.
What are we testing in the second publish?
There was a problem hiding this comment.
In one, the client is the publisher, in the other, the server is.
| serverHandler = server.subscribeCompletionHandler(topic1) | ||
| clientHandler = client.subscribeCompletionHandler(topic2) | ||
|
|
||
| await sleepAsync(FUTURE_TIMEOUT) |
There was a problem hiding this comment.
I have the impression that this isn't needed, comparing to the previous test :)
| WakuMessage(payload: "message1".toBytes(), contentTopic: contentTopicShort), | ||
| ) | ||
| let | ||
| serverResult1 = await serverHandler.waitForResult(FUTURE_TIMEOUT_LONG) |
There was a problem hiding this comment.
why we need a longer wait in this case?
|
|
||
| # When the server publishes a message in the server's subscribed topic | ||
| discard await server.publish( | ||
| some(pubsubTopic1), |
There was a problem hiding this comment.
You did a great job of calculating the value and then hard-coding it above but I wonder if we could use a value obtained from codebase logic instead of using a hard-coded value. That would also help to understand how the pubsub topic is inferred from the content topic
| ) | ||
| let | ||
| serverResult1 = await serverHandler.waitForResult(FUTURE_TIMEOUT) | ||
| clientResult1 = await clientHandler.waitForResult(FUTURE_TIMEOUT) |
There was a problem hiding this comment.
Maybe it is ok to shrink timeout in such error case as it won't complete before! WDYT?
There was a problem hiding this comment.
The FUTURE_TIMEOUT is a 1s const. Do you think it's too much?
| clientResult2 = await clientHandler.waitForResult(FUTURE_TIMEOUT) | ||
|
|
||
| # Then the client receives the message but the server does not | ||
| check serverResult2.isErr() |
There was a problem hiding this comment.
Out of curiousity: Is it possible - a valid scenario - to publish message on topic I'm not subscribed to? Seems to me controversial due I cannot relay message on topic Im not subscribed to. Am I missing sthg?
| WakuMessage(payload: "message2".toBytes(), contentTopic: contentTopic2), | ||
| ) | ||
| let | ||
| serverResult2 = await serverHandler.waitForResult(FUTURE_TIMEOUT_LONG) |
There was a problem hiding this comment.
Should not wait that long (10sec) for something will not occur...
There was a problem hiding this comment.
This might've been copy-paste mistake. I've updated it.
| assertResultOk(await serverHandler1.waitForResult(FUTURE_TIMEOUT)) | ||
| assertResultOk(await clientHandler1.waitForResult(FUTURE_TIMEOUT)) | ||
| check: | ||
| (await serverHandler2.waitForResult(FUTURE_TIMEOUT)).isErr() |
There was a problem hiding this comment.
If I'm not mistaken it is 6 sec wait added here. Can it be shorter or wait parallel somehow and than check ?
There was a problem hiding this comment.
That's a good question. I'm not sure how I'd do this in Nim, perhaps the waitFor allFutures(@[])? But I'm not sure if that behaves like that.
| assertResultOk(await serverHandler2.waitForResult(FUTURE_TIMEOUT)) | ||
| assertResultOk(await clientHandler2.waitForResult(FUTURE_TIMEOUT)) | ||
| check: | ||
| (await serverHandler1.waitForResult(FUTURE_TIMEOUT)).isErr() |
AlejandroCabeza
force-pushed
the
tests/sharding
branch
from
761eff6 to
e1762c4
Compare
* Implement sharding tests.
Description
Implement sharding tests
Changes