Skip to content

Fix test_group_result_not_has_cache for sync backends with native join#5744

Merged
thedrow merged 1 commit into
masterfrom
fix-test_group_result_not_has_cache-for-cache-backend
Oct 24, 2019
Merged

Fix test_group_result_not_has_cache for sync backends with native join#5744
thedrow merged 1 commit into
masterfrom
fix-test_group_result_not_has_cache-for-cache-backend

Conversation

@thedrow

@thedrow thedrow commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

PR #5739 uncovered multiple problems with the cache backend.
This PR should resolve one of them.

PR #5638 fixed the same test case for our async results backends that support native join.
However, it did not fix the test case for sync results backends that support native join.

Unfortunately the fix provided here is inelegant at best.
Further suggestions are welcome.

The provided fix simply fetches the results from ResultSets first.
Afterwards, it batches the fetching of all the results from tasks.

@thedrow
thedrow requested review from a team, auvipy and georgepsarakis September 23, 2019 12:49
@auvipy auvipy added this to the 4.4.0 milestone Sep 23, 2019
@thedrow
thedrow force-pushed the fix-test_group_result_not_has_cache-for-cache-backend branch from b65d976 to 6a7b922 Compare September 23, 2019 16:01
@thedrow
thedrow changed the base branch from master to memcached-integration-tests September 23, 2019 16:03
@thedrow
thedrow changed the base branch from memcached-integration-tests to master September 23, 2019 16:04
@thedrow

thedrow commented Sep 23, 2019

Copy link
Copy Markdown
Contributor Author

Lets hold until we can figure out all the problems in #5739.
This is ready for merge as far as I'm concerned.

@thedrow

thedrow commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

/rebase

@thedrow

thedrow commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

No more rebase bot? :(

…canvas.

PR #5739 uncovered multiple problems with the cache backend.
This PR should resolve one of them.

PR #5638 fixed the same test case for our async results backends that support native join.
However, it did not fix the test case for sync results backends that support native join.
@thedrow
thedrow force-pushed the fix-test_group_result_not_has_cache-for-cache-backend branch from 562a0dc to d51544b Compare October 24, 2019 11:27
@codecov

codecov Bot commented Oct 24, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5744 into master will decrease coverage by 0.09%.
The diff coverage is 42.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #5744     +/-   ##
=========================================
- Coverage   83.39%   83.29%   -0.1%     
=========================================
  Files         144      144             
  Lines       16702    16712     +10     
  Branches     2080     2083      +3     
=========================================
- Hits        13928    13921      -7     
- Misses       2560     2574     +14     
- Partials      214      217      +3
Impacted Files Coverage Δ
celery/backends/base.py 93.25% <42.3%> (-2.89%) ⬇️
celery/contrib/migrate.py 97.9% <0%> (-0.53%) ⬇️
celery/app/amqp.py 94.53% <0%> (-0.33%) ⬇️
celery/canvas.py 93.07% <0%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59c7872...d51544b. Read the comment docs.

@auvipy auvipy left a comment

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.

where is its test?

@thedrow
thedrow merged commit c8d2424 into master Oct 24, 2019
@thedrow
thedrow deleted the fix-test_group_result_not_has_cache-for-cache-backend branch October 24, 2019 11:46
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.

2 participants