moomindani commented on code in PR #70831:
URL: https://github.com/apache/airflow/pull/70831#discussion_r3887844766
##########
providers/databricks/tests/unit/databricks/sensors/test_databricks.py:
##########
@@ -69,15 +69,33 @@ def test_init_statement_id(self):
assert op.warehouse_id == WAREHOUSE_ID
@pytest.mark.parametrize(
- ("kwargs", "match"),
+ ("statement", "statement_id"),
[
- ({"statement": STATEMENT, "statement_id": STATEMENT_ID}, "Cannot
provide both"),
- ({}, "One of either statement or statement_id"),
+ (STATEMENT, STATEMENT_ID),
+ (STATEMENT, ""),
],
)
- def test_statement_combination_validated_at_execute(self, kwargs, match):
- op = DatabricksSQLStatementsSensor(task_id=TASK_ID,
warehouse_id=WAREHOUSE_ID, **kwargs)
- with pytest.raises(AirflowException, match=match):
+ def test_both_statements_included_validated_at_init(self, statement,
statement_id):
+ with pytest.raises(ValueError, match="Cannot provide both"):
+ DatabricksSQLStatementsSensor(
+ statement=statement,
+ statement_id=statement_id,
+ task_id=TASK_ID,
+ warehouse_id=WAREHOUSE_ID,
+ )
+
+ @pytest.mark.parametrize(
+ ("statement", "statement_id"),
+ [
+ (None, None),
+ ("", None),
Review Comment:
Good question, and I think the dead-code version actually weakens the guard
rather than strengthening it.
If the constructor raises, `render_template_fields` and `execute` never run
— so the block asserts only "something in here raised `ValueError`". Move the
check back into `execute()` later and the test still passes, which is exactly
the regression you want it to catch. The stronger form is the opposite: assert
on the constructor **alone**, with nothing after it. Then if the check moves
out of `__init__`, the constructor stops raising and the test fails immediately.
```python
with pytest.raises(ValueError, match="Provide exactly one of statement or
statement_id"):
DatabricksSQLStatementsSensor(
task_id=TASK_ID, warehouse_id=WAREHOUSE_ID,
statement=statement, statement_id=statement_id, dag=dag,
)
```
That also discriminates against the old code, which is what my note was
really asking for. Verified both sides locally with
`render_template_as_native_obj=True`, `statement="SELECT 1"`, `statement_id="{{
None }}"`:
* current main (checks in `execute()`, truthiness): constructs fine, renders
to `statement_id=None`, and `execute()` proceeds to submit the statement — no
exception at all.
* this PR: `ValueError: Cannot provide both statement and statement_id.`
from the constructor.
So a plain `pytest.raises` around the constructor fails before the change
and passes after it, which no current test does.
Two smaller points:
The `dag=...` with `render_template_as_native_obj=True` documents the
scenario but the test does not depend on it — the constructor never renders, so
it behaves the same without the flag. If you want a test where rendering is
load-bearing rather than dead code, that is the *other* half, and it is worth
having as a second test: pass only `statement="{{ None }}"`, construct,
`render_template_fields`, then assert `execute()` raises. I measured that on
this PR's head and it gives `ValueError: One of either statement or
statement_id must be provided.` — the render call there is doing real work, and
the pair of tests then pins each check to its own site.
Also note your `match=` uses "Provide exactly one of statement or
statement_id" while the code currently says "Cannot provide both statement and
statement_id." — that test will fail unless the message change from my other
comment lands too. Worth doing both in the same push.
---
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
