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

(fix) TopicMessageQuery#unsubscribe() attempts to re-subscribe #2582

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

Jexsie
Copy link
Contributor

@Jexsie Jexsie commented Oct 15, 2024

Description:

The unsubscribe method invokes the _call function, which cancels the current active subscription but doesn’t necessarily stop retry attempts, as retries are managed separately in TopicMessageQuery. I have added a check to prevent retries after unsubscribing in the error callback

Screen.Recording.2024-10-09.at.13.29.40.mov

Related issue(s):

Fixes #2222

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@Jexsie Jexsie requested review from a team as code owners October 15, 2024 10:30
@Jexsie Jexsie requested a review from rwalworth October 15, 2024 10:30
@Jexsie
Copy link
Contributor Author

Jexsie commented Oct 15, 2024

continuing from #2571
cc @agadzhalov

@agadzhalov
Copy link
Contributor

Hi @Jexsie the commit is still not verified. I can see the commit is signed but are you sure you've setup your GPG keys properly? You can look at this page for more info (Signing commits). Also I can see in your github profile that your previous commits in other directories (example) are not verified too.

Signed-off-by: Jessy Ssebuliba <[email protected]>
@Jexsie Jexsie force-pushed the topic-subscription-cancel branch from 7a29439 to 83a64d5 Compare October 15, 2024 13:09
@Jexsie
Copy link
Contributor Author

Jexsie commented Oct 15, 2024

thanks alot for the support @agadzhalov. Now i see the verified flag

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.45%. Comparing base (34ffa6b) to head (d74190b).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/topic/TopicMessageQuery.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2582   +/-   ##
=======================================
  Coverage   84.45%   84.45%           
=======================================
  Files         283      283           
  Lines       71038    71046    +8     
=======================================
+ Hits        59993    60002    +9     
+ Misses      11045    11044    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivaylonikolov7
Copy link
Contributor

ivaylonikolov7 commented Oct 16, 2024

Before we can proceed with merging the changes, there is one final task to address. We need to incorporate unit tests for this feature, as the continuous integration (CI) process currently fails without them. As part of our commitment to enhancing code quality, we strive to improve code coverage by adding as many tests as possible whenever new features are implemented.

Could you please create a file named TopicMessageQuery.js within the test/unit directory? The content of this file should be structured as follows:

import { expect } from "chai";

describe("TopicMessageQuery", function () {

    it("should ...", async function () {
    });
});

Additionally, please ensure that the test you write covers lines 441-443. Oone test should suffice for this purpose. If you have any more questions I'd be glad to help.

@Jexsie
Copy link
Contributor Author

Jexsie commented Oct 16, 2024

I see we have the TopicMessageMocking file that seems to be mocking the TopicMessageQuery. Won't creating a new file be some sort of duplication, @ivaylonikolov7?

@ivaylonikolov7
Copy link
Contributor

Yeah, fair point. You can use that.

Signed-off-by: Jessy Ssebuliba <[email protected]>
@Jexsie Jexsie force-pushed the topic-subscription-cancel branch from 79cfcf9 to d74190b Compare October 16, 2024 09:32
Copy link

sonarcloud bot commented Oct 16, 2024

@Jexsie
Copy link
Contributor Author

Jexsie commented Oct 16, 2024

I have added another test case to handle the scenario @ivaylonikolov7.

Copy link
Contributor

@agadzhalov agadzhalov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @Jexsie

@agadzhalov agadzhalov merged commit cfe7be7 into hashgraph:main Oct 25, 2024
9 checks passed
@ivaylonikolov7 ivaylonikolov7 mentioned this pull request Nov 7, 2024
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.

fix: TopicMessageQuery#unsubscribe() attempts to re-subscribe
3 participants