meilisearch / meilisearch-go

Golang wrapper for the Meilisearch API
https://www.meilisearch.com
MIT License
481 stars 81 forks source link

Fix issue with ticker leaking CPU, optimize #523

Closed thisisdev-patrick closed 2 months ago

thisisdev-patrick commented 2 months ago

https://github.com/meilisearch/meilisearch-go/issues/522

Pull Request

Related issue

Fixes #522

What does this PR do?

PR checklist

Please check if your PR fulfills the following requirements:

Thank you so much for contributing to Meilisearch!

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.48%. Comparing base (805c05a) to head (9f1538f).

:exclamation: Current head 9f1538f differs from pull request most recent head 1505660. Consider uploading reports for the commit 1505660 to get more accurate results

Files Patch % Lines
client.go 80.95% 2 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #523 +/- ## ========================================== + Coverage 83.52% 84.48% +0.95% ========================================== Files 10 10 Lines 1584 1598 +14 ========================================== + Hits 1323 1350 +27 + Misses 147 133 -14 - Partials 114 115 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thisisdev-patrick commented 2 months ago

@curquiza can you please rerun the workflow? I've added some ut's for unrelated stuff in this PR, I was a victim of poor previous work :) thanks, it would help us a lot if this fix gets released asap. Thanks in advance!

thisisdev-patrick commented 2 months ago

I cannot see how to test those lines if the tests don't run against a mock server. Both those lines return the error value of the GetTask API call, so I guess this execution path is already tested somewhere else.

curquiza commented 2 months ago

I cannot see how to test those lines if the tests don't run against a mock server. Both those lines return the error value of the GetTask API call, so I guess this execution path is already tested somewhere else.

Yes definitely an issue here. I'm considering turn off codecoverage because bringing more lost of time than real value in my job recently. As a first step I will decrease the % of coverage, if possible. Sorry for the delay. Coming back when I have the time

thisisdev-patrick commented 2 months ago

I could add some codecov ignores with your permission? https://community.codecov.com/t/ignore-lines-with-comments-inside-the-files-expected-misses/2661

curquiza commented 2 months ago

I could add some codecov ignores with your permission? https://community.codecov.com/t/ignore-lines-with-comments-inside-the-files-expected-misses/2661

Yes please 🙏

thisisdev-patrick commented 2 months ago

Sorry I screwed up; those tags were a suggestion by some user.. I'll remove them and try to find a solution; please do not merge yet

curquiza commented 2 months ago

Can I merge @thisisdev-patrick? 😊

thisisdev-patrick commented 2 months ago

yes :)

curquiza commented 2 months ago

doing a release this afternoon!

meili-bors[bot] commented 2 months ago

Build succeeded: