Conversation
The test module calls itertools.count without importing itertools, so every test in it errors at collection and the snowflake provider suite is red on main. The mocked clock is wrong on its own terms too: it jumps 20s per read against a 10s timeout, so the poll loop is already past its deadline on the first check and reports a status it never observed. Pinning the three reads the loop actually makes keeps the timeline explicit, and fails loudly rather than silently shifting if the implementation grows another one.
22c04d6 to
b416667
Compare
Andrushika
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
Nit: maybe assert that DESCRIBE count in the test directly, since the description relies on it.
The pinned clock already makes a second poll impossible, since it would need a fourth monotonic() read, but that is a side effect of the mock's length rather than something the test states. A reader of the timeout case should not have to re-derive it.
Good catch, thanks! I’ll add a direct assertion for the DESCRIBE call count. |
| ) | ||
| @mock.patch("time.sleep") | ||
| @mock.patch("time.monotonic", side_effect=itertools.count(0, 20)) | ||
| @mock.patch("time.monotonic", side_effect=[0, 5, 10]) |
There was a problem hiding this comment.
Sorry, after a closer look I think I was wrong here. The itertools.count in the original suggestion was on purpose, to keep the patched clock from running out if anything else reads time.monotonic during the test: #70103 (comment)
So maybe keep the unbounded counter, just with a step that fits the 10s timeout, e.g. itertools.count(0, 5) (0, 5, 10) plus the missing import itertools. count(0, 20) hits the deadline on the first check, which is the other failure you found.
There was a problem hiding this comment.
Thanks for the clarification! Since this has been addressed in #72709, I’ll close this PR.
There was a problem hiding this comment.
@Andrushika I’m thinking of opening a separate one to add test coverage for #72709, since it currently doesn’t have a regression test. What do you think?
There was a problem hiding this comment.
I would say no harm in adding it, but if we don’t think the extra coverage is necessary, I’d rather avoid opening another PR just to create noise.
There was a problem hiding this comment.
OK, I got it. Thank you so much!
Summary
The operator test module has never run. Two failures stack up:
itertools.countis called without importingitertools, so collection fails and all 45 tests in the module error.Pinning the three clock reads the loop actually makes fixes both: the timeout fires after exactly one
DESCRIBE SERVICE, and an implementation that grows a fourth read fails loudly instead of silently shifting the timeline.The operator needs no change: two real
monotonic()reads are microseconds apart, so the pre-poll deadline check can only trip for a non-positive timeout.Left for a follow-up: the mocked hook returns a constant
RUNNING, soassert_called_once_with("RUNNING")separates "polled at least once" from "never polled" rather than the last observed status from an earlier one. Covering that needs a status sequence and a longer deadline.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines