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

registry (threading): fix broken but passing test #217

Merged
merged 2 commits into from
Sep 1, 2024
Merged

Conversation

x0ul
Copy link
Contributor

@x0ul x0ul commented Aug 29, 2024

Hello, while hacking on your framework I noticed these two tests were passing when they shouldn't have been. I realized that in both cases the tests were missing the actor ref fixture parameters, so no actors were ever added to the registry. The assertions inside the loops over the actor refs retrieved from the registry consequently never ran!

I've added the missing actor refs to both tests, plus some additional asserts to cover that the registry contains the expected actors.

Thanks for writing this excellent library (the code is extremely pleasant to work with, I can learn a lot from this). I hope you find this contribution useful.

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.30%. Comparing base (1f4a1f6) to head (a051078).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #217   +/-   ##
=======================================
  Coverage   93.30%   93.30%           
=======================================
  Files          14       14           
  Lines         553      553           
  Branches       76       76           
=======================================
  Hits          516      516           
  Misses         32       32           
  Partials        5        5           

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

@jodal jodal enabled auto-merge September 1, 2024 21:01
@jodal
Copy link
Owner

jodal commented Sep 1, 2024

Thanks for writing this excellent library (the code is extremely pleasant to work with, I can learn a lot from this). I hope you find this contribution useful.

Thanks, both for the kind words and the fix!

@jodal jodal merged commit a2174d9 into jodal:main Sep 1, 2024
12 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.

2 participants