Added testcase for issue #6437 #6684
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6684 +/- ##
=======================================
Coverage 70.46% 70.46%
=======================================
Files 138 138
Lines 16497 16497
Branches 2074 2074
=======================================
Hits 11625 11625
Misses 4669 4669
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
Hey @thedrow my issue was slightly different. The failing task was inside the group, I'd tweak the task signature to the following |
|
Both test cases fail. I can add yours to mine 🙂. |
|
Hmm, your test case doesn't fail in my example. But sure, feel free to add my test case as well. |
Actually, this passes in our integration test suite. This is the test: def test_error_propagates_from_chord2(self, manager):
try:
manager.app.backend.ensure_chords_allowed()
except NotImplementedError as e:
raise pytest.skip(e.args[0])
sig = add.s(1, 1) | add.s(1) | group(add.s(1), fail.s())
res = sig.delay()
with pytest.raises(ExpectedException):
res.get(timeout=TIMEOUT) |
There was a problem hiding this comment.
FWICT the test case here is equivalent to the setup in your freezelery repo which I minimised a bit in #6437. I was basically mimicking the behaviour of the raise in fetch_details_for_user()
which is in the middle of the chain rather than in the group:
IIRC a task failing at the end the body of a chord is a fairly straightforward failure more and may have always worked back to when I started looking at the canvas code. In any case, it doesn't look like we have a simple test case for task failure in the body of a chord so it can't hurt to add something like test_error_propagates_from_chord2() for the sake of completeness in the test suite.
Interestingly, on a quick read through, test_chord_on_error() appears to actually be a more complicated version of this new test where the final task in what I believe will become the chord headeris the thing that fails. But that test presumably passes, it's not marked xfail... Not sure what to make of that.
This change LGTM, approving as it stands and happy with any change which lands a test like test_error_propagates_from_chord2() as well.
| except NotImplementedError as e: | ||
| raise pytest.skip(e.args[0]) | ||
|
|
||
| sig = add.s(1, 1) | fail.s() | group(add.s(1), add.s(1)) |
There was a problem hiding this comment.
It might be better to construct the chord signature manually for test cases rather than relying on the chain->group promotion behaviour.
There was a problem hiding this comment.
But perhaps that's what makes the bug since test_chord_on_error() passes :o worth making the change if you have the time to see if we can narrow down what's breaking
There was a problem hiding this comment.
You're right, it times out when the failing task is in the middle of the chain. I'm sorry for the confusion @thedrow
| except NotImplementedError as e: | ||
| raise pytest.skip(e.args[0]) | ||
|
|
||
| sig = add.s(1, 1) | fail.s() | group(add.s(1), add.s(1)) |
There was a problem hiding this comment.
should we also add si [immutable signature] in the test suit as well?
There was a problem hiding this comment.
I don't think it matters in this case.
|
Before we merge this, let's add all the examples we can think of. |
|
This pull request fixes 1 alert when merging 8095953 into fc55f2a - view on LGTM.com fixed alerts:
|
Since the first case you added here times out, I believe my MRTC from #6437 is already covered. I don't have anything else to add since the second case you added yesterday covers the passing behaviour of the tail fail discussed above as well. lgtm 👍 |
* Added testcase for issue celery#6437. * Add second test case.
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
This reproducible minimal test case demonstrates the issue in #6437.
Per project policy, we prefer to have these test cases in our integration test suite so that we'll know if they were accidentally fixed (so that @auvipy won't have to ask).
@maybe-sybr @emanuelmd if you can minimize this further, please go ahead :)