shahar1 commented on code in PR #70831:
URL: https://github.com/apache/airflow/pull/70831#discussion_r3694875677
##########
providers/databricks/tests/unit/databricks/sensors/test_databricks.py:
##########
@@ -68,16 +68,18 @@ def test_init_statement_id(self):
assert op.statement_id == STATEMENT_ID
assert op.warehouse_id == WAREHOUSE_ID
- @pytest.mark.parametrize(
- ("kwargs", "match"),
- [
- ({"statement": STATEMENT, "statement_id": STATEMENT_ID}, "Cannot
provide both"),
- ({}, "One of either statement or statement_id"),
- ],
- )
- 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):
+ with pytest.raises(ValueError, match="Cannot provide both"):
+ DatabricksSQLStatementsSensor(
+ statement=STATEMENT,
+ statement_id=STATEMENT_ID,
+ task_id=TASK_ID,
+ warehouse_id=WAREHOUSE_ID,
+ )
Review Comment:
The `is not None` polarity is the headline of this PR, and nothing here
exercises it. `STATEMENT` and `STATEMENT_ID` are both non-empty strings, so a
plain `if statement and statement_id:` truthiness check passes this test
identically — meaning the test does not fail without the change it is meant to
cover, which is the bar in [Testing
Standards](https://github.com/apache/airflow/blob/main/CLAUDE.md#testing-standards)
("every test must fail without the PR's change").
The distinguishing case is the empty string — exactly the one called out as
the reason for the polarity in the first place. Adding it back as a parametrize
keeps the split-by-lifecycle naming intact:
```suggestion
@pytest.mark.parametrize(
("statement", "statement_id"),
[
(STATEMENT, STATEMENT_ID),
(STATEMENT, ""),
],
)
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,
)
```
The second case fails on `main` and passes with this PR, which is the
property that was missing.
##########
providers/databricks/src/airflow/providers/databricks/sensors/databricks.py:
##########
@@ -107,8 +110,7 @@ def _get_hook(self, caller: str) -> DatabricksHook:
)
def execute(self, context: Context):
- if self.statement and self.statement_id:
- raise AirflowException("Cannot provide both statement and
statement_id.")
+ # Handle the scenario where both statement and statement_id are not set
Review Comment:
Minor, non-blocking. This comment and its counterpart above the `__init__`
check both restate the condition on the line beneath them, which
[AGENTS.md](https://github.com/apache/airflow/blob/main/CLAUDE.md#coding-standards)
asks us to avoid ("do not write narrating comments that restate the next
line").
The thing that genuinely warrants a comment here is the part the code cannot
say for itself: *why* one half of this validation lives in `__init__` and the
other half lives in `execute()`. That asymmetry is deliberate and load-bearing,
and it is precisely what has been getting reverted back and forth across this
series — a reader arriving cold will otherwise read it as an oversight and
"fix" it again.
```suggestion
# Both fields are templated, so "neither resolves to a value" is
only knowable
# after rendering — __init__ cannot catch it. The both-provided case
is a pure
# provision error and is checked there instead.
```
Dropping the `# Handle the scenario where both statement and statement_id
are set` line above the `__init__` check would round this out — with the
rationale recorded here, that one carries nothing the `if` does not already say.
--
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]
