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

Added client.StopTime(), to expose disconnected #400

Merged
merged 2 commits into from
May 19, 2024

Conversation

snej
Copy link
Contributor

@snej snej commented May 10, 2024

My persistence hook needs to know what time a client disconnected. The field Client.State.disconnected has that information but it's not exposed in the API, so I added a simple public accessor method Client.StopTime().

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9038259883

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 98.623%

Totals Coverage Status
Change from base Build 8892157277: 0.001%
Covered Lines: 6160
Relevant Lines: 6246

💛 - Coveralls

@werbenhu
Copy link
Member

werbenhu commented May 15, 2024

@snej It would be better to add a test case to Client.StopTime().

I added a check to TestClientStop(), both before and after stopping.

I also noticed a race condition in the test (comparing a time against
time.Now) and fixed it to allow a one-second discrepancy.
@snej
Copy link
Contributor Author

snej commented May 15, 2024

Added a check to TestClientStop(). I also noticed a race condition in that test (comparing a time against
time.Now) and fixed it to allow a one-second discrepancy.

Copy link
Collaborator

@mochi-co mochi-co left a comment

Choose a reason for hiding this comment

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

Nice addition! Thank you!

@mochi-co mochi-co merged commit cc3f827 into mochi-mqtt:main May 19, 2024
3 checks passed
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