moomindani commented on code in PR #70831:
URL: https://github.com/apache/airflow/pull/70831#discussion_r3810490154
##########
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, ""),
Review Comment:
Worth adding `("", "")` here so whichever behaviour you settle on for the
both-empty case is pinned down — right now nothing covers it.
##########
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:
The case that actually motivates the polarity change is not covered
anywhere: a *provided* field that renders to `None` under
`render_template_as_native_obj=True`, which must not slip past the exclusivity
check. Without it, a later well-meaning move back to a truthiness check in
`execute()` would pass CI.
##########
providers/databricks/src/airflow/providers/databricks/sensors/databricks.py:
##########
@@ -69,9 +69,11 @@ def __init__(
include_airflow_query_tags: bool = True,
**kwargs,
):
- # Handle the scenario where either both statement and statement_id are
set/not set
+ if statement is not None and statement_id is not None:
Review Comment:
With both fields `""` this raises "Cannot provide both" even though nothing
was really provided — measured on the patched build, and `main` reports the
accurate "One of either statement or statement_id must be provided." from
`execute()` for the same input.
The `is not None` polarity is right and I would not weaken it; the cheapest
fix is a message that holds for either failure, e.g. `"Provide exactly one of
statement or statement_id."`
--
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]
