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

FetchCommunity: various fixes and improvements #4509

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

igor-sirotin
Copy link
Collaborator

@igor-sirotin igor-sirotin commented Dec 22, 2023

Closes #4496
Closes #4496

Main changes

  1. Temporal fix for bug: sort order ignored in store nodes waku-org/nwaku#2317 bug.
    I made the manager to always fetch all envelopes in the batch (no stop when community was found).
    These lines should be removed/reverted when the original bug is fixed:

    // TODO: remove these 2 when fixed: https://github.com/waku-org/nwaku/issues/2317
    opts = append(opts, WithStopWhenDataFound(false))
    opts = append(opts, WithInitialPageSize(defaultStoreNodeRequestPageSize))

    s.Require().Equal(2, stats.FetchedPagesCount) // TODO: revert to 3 when fixed: https://github.com/waku-org/nwaku/issues/2317

  2. Check if a desired community already exists in the database and require the fetched community Clock to be higher.
    If no newer information was fethed, nil is returned, even if the community exists in the database

  3. Implement StoreNodeRequestManager options:

  • WaitForResponse
  • StopWhenDataFound
  • InitialPageSize
  • FurtherPageSize
    This allows a better control over the request. And made it possible to implement the fix (1) tidy.

4. It seems to me that the TestRequestMultipleCommunities is now less flaky, but still fails at about 1% cases.
4. UPD: Fixes TestRequestMultipleCommunities flaky test

  1. Extracted DefaultMailserversByFleet function

  2. Some other minor refactoring and logging

Test improvements

  1. Added a mechanism to wait for the test store node to receive the envelopes.
    This was previously leading to bugs, the case is tested with TestSequentialUpdates

  2. Implemented a test to check the fetched envelopes order: TestRequestCommunityEnvelopesOrder
    This is to address bug: sort order ignored in store nodes waku-org/nwaku#2317 bug, to ensure that our code behaves correctly (assuming that go-waku is also working correctly).

  3. Implemented a TestFetchRealCommunity
    it allows to fetch community from all store nodes in a given fleet and check community state on each store node.

@status-im-auto
Copy link
Member

status-im-auto commented Dec 22, 2023

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 7f7cec2 #1 2023-12-22 20:34:07 ~3 min linux 📦zip
✔️ 7f7cec2 #1 2023-12-22 20:35:18 ~4 min ios 📦zip
✔️ 7f7cec2 #1 2023-12-22 20:36:07 ~5 min android 📦aar
✖️ 7f7cec2 #1 2023-12-22 20:52:31 ~21 min tests 📄log
✖️ 7f7cec2 #2 2023-12-22 22:53:25 ~31 min tests 📄log
✔️ 7f7cec2 #3 2023-12-25 14:13:41 ~37 min tests 📄log

Copy link
Contributor

@mprakhov mprakhov left a comment

Choose a reason for hiding this comment

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

LGTM

@igor-sirotin igor-sirotin merged commit 195982c into develop Dec 27, 2023
7 checks passed
@igor-sirotin igor-sirotin deleted the chore/test-request-community-from-fleet branch December 27, 2023 13:53
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.

FetchCommunity doesn't get a new community description after adding permissions
3 participants