Fix eager tasks does not populate name field#8486
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8486 +/- ##
==========================================
+ Coverage 87.44% 87.46% +0.02%
==========================================
Files 148 148
Lines 18491 18512 +21
Branches 3156 3163 +7
==========================================
+ Hits 16169 16192 +23
+ Misses 2033 2032 -1
+ Partials 289 288 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
auvipy
left a comment
There was a problem hiding this comment.
I can see the difference here from the previous PR. but shouldn't the newer approach also got proper test coverage? You can see my comment of Integration test on the previous PR and we ignored and caught with the regression, right? so It would be better to have as much test case as possible. thanks for handling this again!
|
I'll try to add some tests but I think essentially the problem with the first approach was that it introduced a constructor change with a new required variable. As I understand the functionality did no cause any issues it only broke custom code parts for users which was an unexpected change in a patch version and it was not documented in the release notes as I did not expect that either. I think this approach can be refactored later to be similar to the first one, which is more consistent coding style wise, in a bigger version bump. |
|
OK will be waiting for the missing/additional tests |
|
it would be really great if we can add some test and merge this PR ASAP :) |
|
Sorry I didn't have time, I'll try to do it as soon as possible. :) |
b630c2e to
120b018
Compare
|
Hey @auvipy, Sorry it took me so long but I added some tests. As all other tests pass without modification, I think it should be a safe change now. Let me know what you think. Thanks! |
|
should we consider the OLD test https://github.com/celery/celery/pull/8383/files#diff-c63df79f676057328c4244cd9d26001610598b4bc772d503e5120a89291e46fc as well in the chord test? what about integration test to check nothing breaks? these are not mandatory but nice to have |
|
The modification in the chord test was only necessary because the change was mandatory. Now I think it's better not to change it because it validates that it is now not a breaking change. The functions inside celery populates the attribute so only custom functions made by users need to be adjusted. I added one more check to make sure the name field is still populated inside EagerResults even when they are called through a normal task flow. I don't think an integration test is necessary as most aspects are covered through the unit tests and I only see a really few patterns to integration test eager tasks. |
|
yeah the new extra line looks great. thanks a lot |
Note: Before submitting this pull request, please review our contributing guidelines.
Description
My previous attempt (#8383) introduced a breaking change. This PR however should only contain the bear minimum to fix the issue without interfering with the current behaviour.
Fixes #7715, fixes #8472