You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "rjgoyln (via GitHub)" <gi...@apache.org> on 2026/09/08 12:02:03 UTC
[PR] Make the Snowpark container job operator tests collect and pass [airflow]
rjgoyln opened a new pull request, #72701:
URL: https://github.com/apache/airflow/pull/72701
## Summary
The operator test module has never run. Two failures stack up:
- `itertools.count` is called without importing `itertools`, so collection fails and all 45 tests in the module error.
- With the import in place, the timeout test's mocked clock jumps 20s per read against a 10s timeout, so the poll loop gives up before describing the service once and logs no status.
Stepping the mocked clock by the operator's poll interval leaves room for exactly one poll — what the loop does between deadline checks on a real clock — so the test asserts the last observed status, as its name says. 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.
## Tests
65 passed across the three `snowpark_containers` test modules.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 5)
Generated-by: Claude Code (Opus 5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on code in PR #72701:
URL: https://github.com/apache/airflow/pull/72701#discussion_r3958765982
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review Comment:
OK, I got it. Thank you so much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on code in PR #72701:
URL: https://github.com/apache/airflow/pull/72701#discussion_r3958408079
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review Comment:
Thanks for the clarification! Since this has been addressed in #72709, I’ll close this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on PR #72701:
URL: https://github.com/apache/airflow/pull/72701#issuecomment-5585556011
> LGTM, thanks for the fix!
> Nit: maybe assert that DESCRIBE count in the test directly, since the description relies on it.
Good catch, thanks! I’ll add a direct assertion for the DESCRIBE call count.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on code in PR #72701:
URL: https://github.com/apache/airflow/pull/72701#discussion_r3958274117
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review 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: https://github.com/apache/airflow/pull/70103#discussion_r3956945221
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.
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review Comment:
cc @potiuk since it was your suggestion in #70103, in case I read it wrong. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "Andrushika (via GitHub)" <gi...@apache.org>.
Andrushika commented on code in PR #72701:
URL: https://github.com/apache/airflow/pull/72701#discussion_r3958624782
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln closed pull request #72701: Make the Snowpark container job operator tests collect and pass
URL: https://github.com/apache/airflow/pull/72701
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the Snowpark container job operator tests collect and pass [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on code in PR #72701:
URL: https://github.com/apache/airflow/pull/72701#discussion_r3958504845
##########
providers/snowflake/tests/unit/snowflake/operators/test_snowpark_containers.py:
##########
@@ -173,7 +173,7 @@ def test_poll_waits_through_pending_then_done(self, mock_hook_cls, mock_sleep):
[(True, True), (False, False)],
)
@mock.patch("time.sleep")
- @mock.patch("time.monotonic", side_effect=itertools.count(0, 20))
+ @mock.patch("time.monotonic", side_effect=[0, 5, 10])
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org