Skip to content

Conversation

@AHReccese
Copy link
Member

@AHReccese AHReccese commented Aug 5, 2025

Reference Issues/PRs

Previously when I was adding WinMM engine, I saw some areas in the implementation which needed some minor improvement and in this PR I'm applying them.

What does this implement/fix? Explain your changes.

Any other comments?

Local tests on OSs

  • macOS
    • Sonoma
  • Windows
    • Windows 11
  • Linux
    • Ubuntu 20.04

@AHReccese AHReccese added this to the nava v0.8 milestone Aug 5, 2025
@AHReccese AHReccese self-assigned this Aug 5, 2025
@codecov
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

❌ Patch coverage is 46.15385% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.84%. Comparing base (77ed428) to head (f9131cf).

Files with missing lines Patch % Lines
nava/thread.py 46.16% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #69      +/-   ##
==========================================
- Coverage   96.37%   93.84%   -2.53%     
==========================================
  Files           4        4              
  Lines         220      227       +7     
  Branches       30       30              
==========================================
+ Hits          212      213       +1     
- Misses          5       11       +6     
  Partials        3        3              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AHReccese AHReccese requested a review from sadrasabouri August 11, 2025 06:50
@AHReccese AHReccese marked this pull request as ready for review August 11, 2025 06:50
Copy link
Member

@sadrasabouri sadrasabouri left a comment

Choose a reason for hiding this comment

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

@AHReccese Can you add tests to increase the coverage? This looks a little bit suspicious to me and I don't want to merge it without tests.
I need @sepandhaghighi to also review this PR. I can't review it on my own.

After this PR I we're good for a release. We haven't released for a long time.

@sadrasabouri
Copy link
Member

@AHReccese a friendly reminder on my comment for this.

@AHReccese
Copy link
Member Author

Thank you, Sadra. Adding tests to cover the related corner cases is a bit challenging, but I’ll think it through and we can also discuss it in our meeting. I’ll explain the changes to Sepand, and together we can find a way forward.
@sadrasabouri @sepandhaghighi

@AHReccese
Copy link
Member Author

AHReccese commented Aug 19, 2025

Feedback from Sepand:

  • Make the exception type more coarse-grained
  • [Future] Use pytest Mock to catch more coverage in the next PRs

@sadrasabouri
Copy link
Member

sadrasabouri commented Sep 2, 2025

Feedback from Sepand:

  • Make the exception type more coarse-grained
  • [Future] Use pytest Mock to catch more coverage in the next PRs

Dear @AHReccese, this is a friendly reminder for this PR.
Also include @sepandhaghighi for this review.

…ceptions and improve clarity in comments. Ensure `_play_process` is set to `None` after termination attempts.
@AHReccese
Copy link
Member Author

@sepandhaghighi @sadrasabouri
Hi folks, I'm sorry for the latency regarding this PR,
I've considered a coarse-grained exception type in the stop function, in the next PR, I will add more mock tests to increase coverage. For now let's wrap up this PR and then I will open the test PR promptly right after. I will try to cover as much as possible but as you know, creating such scenarios to cover associated exceptions might be a little bit tricky but I will do it as much as possible.

Copy link
Member

@sadrasabouri sadrasabouri left a comment

Choose a reason for hiding this comment

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

Looks good but I would like to have @sepandhaghighi 's review on this before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants