Skip to content
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: unit test for duplicate message push #2852

Merged
merged 9 commits into from
Jun 28, 2024

Conversation

darshankabariya
Copy link
Contributor

It's related to #2320

It's test duplicate message push from filter service node to filter client

Copy link

github-actions bot commented Jun 26, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2852

Built from 7749908

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I haven't checked in in deep detail but we need to have fast tests :)
Cheers

len(messages1.data) == 1

# Pause execution for 2 minutes to test TimeCache functionality of service node
sleep(120000)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, unfortunately we can't wait for so long in a single test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, we should make it configurable and just configure a lower value for testing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darshankabariya : I think you need to extend here the mountFilter with a defaulted TTL argument that can be passed to WakuFilter.new...

maxFilterCriteriaPerPeer: uint32 = filter_subscriptions.MaxFilterCriteriaPerPeer,

Than in the test code in the setup part, at testSetup.serviceNode.mountFilter add a test TTL delay as 1.seconds at max, but maybe 500.milliseconds also fine.

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust mountFilter with the dup check TTL config and than you are able to configure this test's timeout to be short.

len(messages1.data) == 1

# Pause execution for 2 minutes to test TimeCache functionality of service node
sleep(120000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darshankabariya : I think you need to extend here the mountFilter with a defaulted TTL argument that can be passed to WakuFilter.new...

maxFilterCriteriaPerPeer: uint32 = filter_subscriptions.MaxFilterCriteriaPerPeer,

Than in the test code in the setup part, at testSetup.serviceNode.mountFilter add a test TTL delay as 1.seconds at max, but maybe 500.milliseconds also fine.

@darshankabariya darshankabariya force-pushed the unit_test_rest_filter branch from 1dfd377 to 9cfaf90 Compare June 27, 2024 20:08
@darshankabariya
Copy link
Contributor Author

darshankabariya commented Jun 27, 2024

Please adjust mountFilter with the dup check TTL config and than you are able to configure this test's timeout to be short.

I’ve been updating the tests based on your suggestions. However, I encountered an issue with await testSetup.serviceNode.mountFilter(cacheTTL = 1.seconds). I just need to override this individual parameter, but it's showing a type mismatch (one is TimeInterval and the other is Duration) i try many thing but issue is persistence. In my local setup, the tests are not running correctly, but I will still push the changes to the branch. Sometimes my local environment behaves differently from the remote setup. Let’s see what happens. If you find anything wrong, please let me know.

@NagyZoltanPeter
Copy link
Contributor

Please adjust mountFilter with the dup check TTL config and than you are able to configure this test's timeout to be short.

I’ve been updating the tests based on your suggestions. However, I encountered an issue with await testSetup.serviceNode.mountFilter(cacheTTL = 1.seconds). I just need to override this individual parameter, but it's showing a type mismatch (one is TimeInterval and the other is Duration) i try many thing but issue is persistence. In my local setup, the tests are not running correctly, but I will still push the changes to the branch. Sometimes my local environment behaves differently from the remote setup. Let’s see what happens. If you find anything wrong, please let me know.

Yeah, saw and fixed that. It was a wrong import std/times instead of chronos/timer. Now it compiles I think.

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be ok now with 1 sec delay for test.

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compile error is fixed but test fails unfortunately:

2024-06-28T00:09:14.7098245Z   /home/runner/work/nwaku/nwaku/build/all_tests_waku 'Waku v2 Rest API - Filter V2::duplicate message push to filter subscriber ( sleep in between )'
2024-06-28T00:09:14.7099205Z ----------------------------------------------------------------
2024-06-28T00:09:14.7099981Z     /home/runner/work/nwaku/nwaku/tests/wakunode_rest/test_rest_filter.nim(462, 26): Check failed: len(messages2.data) == 1
2024-06-28T00:09:14.7100699Z     len(messages2.data) was 0
2024-06-28T00:09:14.7100903Z 
2024-06-28T00:09:14.7101289Z   [FAILED ] (  0.02s) duplicate message push to filter subscriber ( sleep in between )�[0m
2024-06-28T00:09:14.7101714Z 
2024-06-28T00:09:14.7101870Z (0.09s)   Waku v2 Rest API - Filter V2```

@darshankabariya darshankabariya force-pushed the unit_test_rest_filter branch from 13f0baf to 8a68bd4 Compare June 28, 2024 05:48
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as the tests pass it is very well done!!!

len(messages1.data) == 1

# Pause execution for 2 minutes to test TimeCache functionality of service node
sleep(1000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it was the issue!
Maybe to change it for a more commonly used version:

Suggested change
sleep(1000)
await sleepAsync(1.seconds)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side note: using async sleep will not stop the whole thread so async tests can run meanwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await sleepAsync(1.seconds)

oh that's awesome !

postMsgResponse1.data == "OK"
len(messages1.data) == 1

# Pause execution for 2 minutes to test TimeCache functionality of service node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Pause execution for 2 minutes to test TimeCache functionality of service node
# Pause execution for 1 second to test TimeCache functionality of service node

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks! Make sure Zoltán's comments are considered before merging :)

@darshankabariya
Copy link
Contributor Author

LGTM! Thanks! Make sure Zoltán's comments are considered before merging :)

Sure !

@darshankabariya darshankabariya merged commit 31c632e into master Jun 28, 2024
8 of 10 checks passed
@darshankabariya darshankabariya deleted the unit_test_rest_filter branch June 28, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants