ahilashsasidharan commented on code in PR #70831:
URL: https://github.com/apache/airflow/pull/70831#discussion_r3882872833
##########
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:
I believe the polarity change should be covered, but I agree that the
`render_template_as_native_obj=True` scenario isn't covered.
I am not sure how to cover it cleanly though currently I've settled on
potentially implementing something like the below testcase if it passes all
checks. From my understanding (AI-assisted) we need to explicitly render
templates after setting the value of it in the DAG so it requires leaving dead
code at the bottom within the pytest.raise block to render templates that will
never run since currently `__init__` catches the error:
```python
context = {"dag": dag}
op.render_template_fields(context)
op.execute(context)
```
Without it someone moving the code block to execute might rewrite the test
without rendering the templates which would still allow it to pass CI and might
be missed by a reviewer. With this code they would need to explicitly remove
the render which is easier to catch in a review.
I'll also add a comment mentioning intent of the test for anyone that is
reading the code, but not sure if there is a cleaner way to implement this
beyond that which you might have in mind.
```python
@pytest.mark.parametrize("statement,statement_id", [
("{{ None }}", STATEMENT_ID), # statement would render to None
(STATEMENT, "{{ None }}"), # statement_id would render to None
("{{ None }}", "{{ None }}"), # both would render to None
])
def test_both_provided_one_as_template_raises_at_init(self, statement,
statement_id):
dag = DAG(
dag_id="test_native_obj_dag",
start_date=timezone.datetime(2025, 1, 1),
schedule=None,
render_template_as_native_obj=True,
)
with pytest.raises(ValueError, match="Provide exactly one of statement
or statement_id"):
op = DatabricksSQLStatementsSensor(
task_id=TASK_ID,
warehouse_id=WAREHOUSE_ID,
statement=statement,
statement_id=statement_id,
dag=dag,
)
context = {"dag": dag}
op.render_template_fields(context)
op.execute(context)
```
--
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]
