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

ChainMap observers fix #8305

Merged
merged 2 commits into from Jun 14, 2023
Merged

ChainMap observers fix #8305

merged 2 commits into from Jun 14, 2023

Conversation

shahar-lev
Copy link
Contributor

Observers should not be shared across different instances. Aside from unwanted behavior, this can lead to object leaks (like celery app objects not being garbage collected).

shahar-lev and others added 2 commits June 8, 2023 16:14
Observers should not be shared across different instances.
Aside from unwanted behavior, this can lead to object leaks (like
celery app objects not being garbage collected).
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (e540a8d) 87.16% compared to head (ba978da) 87.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8305      +/-   ##
==========================================
- Coverage   87.16%   87.15%   -0.01%     
==========================================
  Files         148      148              
  Lines       18469    18469              
  Branches     3097     3148      +51     
==========================================
- Hits        16098    16097       -1     
- Misses       2092     2094       +2     
+ Partials      279      278       -1     
Flag Coverage Δ
unittests 87.12% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/utils/collections.py 85.65% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@auvipy auvipy self-requested a review June 9, 2023 09:29
callback = Mock()
a.bind_to(callback)
b.update(x=1)
callback.assert_not_called()
Copy link
Member

Choose a reason for hiding this comment

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

can we lean to pytest like syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there an alternative to assert_not_called and assert_called_once_with in pytest?
I see they are used extensively in celery unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

ugh sorry my mistake!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you need anything else from me?

Copy link
Member

Choose a reason for hiding this comment

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

is it possible to increase test coverage and if possible to add integration tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's weird that I reduced coverage by this commit - It's just a bugfix with a new unit test that checks it actually fixed the bug (I didn't add any new "logic" to the code itself).
It also feels like a low-level bug that doesn't suit an integration test.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

codecov sometimes do mistakes :D

@auvipy auvipy requested a review from Nusnus June 14, 2023 06:21
@auvipy auvipy merged commit 58c851e into celery:main Jun 14, 2023
27 of 29 checks passed
@auvipy auvipy added this to the 5.3.x milestone Jun 14, 2023
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.

None yet

2 participants