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: debug canary tests #494

Merged
merged 2 commits into from
Sep 5, 2024
Merged

chore: debug canary tests #494

merged 2 commits into from
Sep 5, 2024

Conversation

rishtigupta
Copy link
Contributor

@rishtigupta rishtigupta commented Sep 5, 2024

PR Description:

**Test: [topics-service] "Publishes and receives detailed subscription items":**
The test is failing because it's comparing the first received value (aaa) against the second expected value ([1, 2, 3]), which is causing the mismatch. This mismatch is likely because we have extra non-published items, such as a heartbeat, being included in the receivedItems array. The key issue is that we are not filtering out non-TopicItem events (e.g., TopicHeartbeat, TopicDiscontinuity) before doing the comparison with expectedValues.

Hence, this commit compares only the received TopicItem values to the expectedValues.

**Test: [cache-service][scalar-tests] "accurately reports the remaining TTL for a key":**
My hunch is that it can be either of the 3 issues:

  1. The TTL might not be exactly what is expected due to small timing discrepancies between setting the TTL and when it is retrieved. This might happen due to network latency or minor processing delays.
  2. If ItemGetTtl is called immediately after setting the key, there may be too little difference between the ttl and RemainingTtl(), causing the comparison to fail.
  3. If the system clocks between the client and server are not perfectly synchronized, it can cause discrepancies (maybe?)

I also found that gingko has an inbuilt BeNumerically matcher, which is designed specifically to compare numerical values. So, instead of manual boolean checker, using inbuilt matcher to compare original ttl with remaining ttl.

Verified

This commit was signed with the committer’s verified signature.
h3ndrk Hendrik
@rishtigupta rishtigupta force-pushed the chore/update-test-canary branch from e47c2e8 to 6a471ed Compare September 5, 2024 21:43
@rishtigupta rishtigupta marked this pull request as ready for review September 5, 2024 21:46
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

handful of questions/comments.

Nice find on the topic publish test!

Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

@rishtigupta rishtigupta merged commit 8a5d399 into main Sep 5, 2024
6 checks passed
@rishtigupta rishtigupta deleted the chore/update-test-canary branch September 5, 2024 22:41
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.

None yet

3 participants