Skip to content

Fix recursive result parents on group in middle of chain#8903

Merged
Nusnus merged 2 commits intocelery:mainfrom
beneltayar:fix-recursive-result-parents
Mar 10, 2024
Merged

Fix recursive result parents on group in middle of chain#8903
Nusnus merged 2 commits intocelery:mainfrom
beneltayar:fix-recursive-result-parents

Conversation

@beneltayar
Copy link
Contributor

Description

This fixes issue #8890
Which is a bug when trying to freeze a chain that contains a group and more than 2 tasks after it
Also added a test case

@codecov
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.48%. Comparing base (79ec40a) to head (e37740a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8903      +/-   ##
==========================================
+ Coverage   81.47%   81.48%   +0.01%     
==========================================
  Files         150      150              
  Lines       18681    18686       +5     
  Branches     3192     3193       +1     
==========================================
+ Hits        15221    15227       +6     
  Misses       3168     3168              
+ Partials      292      291       -1     
Flag Coverage Δ
unittests 81.46% <100.00%> (+0.01%) ⬆️

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

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

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

thanks for the proposed fix with unit test. should we try to add extra integration tests for this as well? it's not mandatory though

@Nusnus
Copy link
Member

Nusnus commented Mar 10, 2024

thanks for the proposed fix with unit test. should we try to add extra integration tests for this as well? it's not mandatory though

You were reading my mind @auvipy .
Although IMHO I'd say it is mandatory.

But let's see first if the CI passed successfully with the current tests.

Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

Regardless, the fix does look good

@beneltayar
Copy link
Contributor Author

@Nusnus I've added the integration test, but now one of the checks is not successful, I don't think it has anything to do with me, it's getting connection refused from the Redis.

@Nusnus Nusnus merged commit 62a1a50 into celery:main Mar 10, 2024
@Nusnus
Copy link
Member

Nusnus commented Mar 10, 2024

@Nusnus I've added the integration test, but now one of the checks is not successful, I don't think it has anything to do with me, it's getting connection refused from the Redis.

Indeed, unrelated.

Merged!

Good work, thank you.

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.

Error "Recursive result parents" on some tasks trees

4 participants