Fix logic bug in create_schedule() re. backfills #693
Conversation
Don't ask for immediate trigger when a backfill is requested, and don't ask for a backfill when an immediate trigger is requested. Closes #678.
|
In general I like us to always create a failing test when fixing a bug. Have you looked into how difficult that would be to do here? |
|
I am unsure if this is worth adding tests for, but I can. I'd tried to tweak test_schedule_backfill() but that didn't work in the way I expect, so I'd have to spend some time actually learning how schedules work and writing a new test, I think. |
lol, we crossed on the wire. Yes, I think it would take me a couple (more) hours since I'm not really familiar with schedules. (That's not counting the hour or two I've already spent trying to tweak an existing test to validate this as well.) |
|
Does any SDK have tests for this functionality? If so, we could appeal to an argument for coverage by analogy to the other SDK. (Also it might teach us how to provide coverage here.) |
I don't believe this is relevant in any other SDK; TypeScript exposes schedule creation in a different way. Go is kinda similar but does not test this so far as I can see. Java also has a completely different approach using builders. |
|
hm, looking at some of the CI failures (which I did not see running locally that I can recall), I'm wondering if the existing tests actually ARE sufficient but were just wrong. I was hoping this would be straightforward but I think I'll need to dig more anyway. |
| if input.backfill | ||
| else None, |
There was a problem hiding this comment.
| if input.backfill | |
| else None, |
Not really needed tbh, but not a big deal
There was a problem hiding this comment.
(Also usual Python formatting style is to place parens around xxx if cond else yyy expressions when used as arguments so that ruff indents the expression:
initial_patch = temporalio.api.schedule.v1.SchedulePatch(
trigger_immediately=(
temporalio.api.schedule.v1.TriggerImmediatelyRequest(
overlap_policy=temporalio.api.enums.v1.ScheduleOverlapPolicy.ValueType(
input.schedule.policy.overlap
),
)
if input.trigger_immediately
else None
),
backfill_request=(
[b._to_proto() for b in input.backfill] if input.backfill else None
),
)
|
OK, that actually didn't take as long as I thought. So it turns out test_schedule_backfill() actually was testing for the presence of the bug (it was counting an extra action which no longer happens if @dandavison This should address your (and my) concern about preventing regressions in the future. |
There was a problem hiding this comment.
Nice, LGTM! I see (desired) test failures when I switch to main and checkout the tests from this branch:
FAILED tests/test_client.py::test_schedule_backfill - assert 3 in [1, 2]
FAILED tests/test_client.py::test_schedule_search_attribute_update - AssertionError: timed out waiting for equal, asserted against last value of 1
Are both those expected?
|
|
Right (that's what I meant by desired failures). When reviewing I usually run the tests from the PR with the feature changes reverted to ensure that there is a test failure on
OK great, I must have been having bad luck with that one. |
Don't ask for immediate trigger when a backfill is requested, and don't ask for a backfill when an immediate trigger is requested.
Closes #678.