Skip to content

ChainMap observers fix#8305

Merged
auvipy merged 2 commits intocelery:mainfrom
shahar-lev:main
Jun 14, 2023
Merged

ChainMap observers fix#8305
auvipy merged 2 commits intocelery:mainfrom
shahar-lev:main

Conversation

@shahar-lev
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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.

2 participants