search

temporalio / sdk-python

Temporal Python SDK
MIT License
472 stars 75 forks source link

Fix logic bug in create_schedule() re. backfills #693

Closed josh-berry closed 3 days ago

josh-berry commented 4 days ago

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.

dandavison commented 4 days ago

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?

josh-berry commented 4 days ago

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.

josh-berry commented 4 days ago

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?

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.)

dandavison commented 4 days ago

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.)

josh-berry commented 4 days ago

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.

josh-berry commented 4 days ago

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.

josh-berry commented 3 days ago

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 trigger_immediately is False). So I've updated the action counts accordingly and added some comments explaining how we arrived at the total action counts in the assertions.

@dandavison This should address your (and my) concern about preventing regressions in the future.

josh-berry commented 3 days ago

schedule_backfills is yes, that's how we know we've fixed this bug. :) The search-attribute update one appears to be an unrelated intermittent failure.

dandavison commented 3 days ago

schedule_backfills is yes, that's how we know we've fixed this bug. :)

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 main as there should be.

The search-attribute update one appears to be an unrelated intermittent failure.

OK great, I must have been having bad luck with that one.