potiuk commented on code in PR #72952:
URL: https://github.com/apache/airflow/pull/72952#discussion_r4057531991
##########
providers/google/tests/unit/google/cloud/transfers/test_http_to_gcs.py:
##########
@@ -125,3 +128,23 @@ def test_execute_returns_destination_uris(self, http_hook,
gcs_hook):
expected_uris = f"gs://{TEST_BUCKET}/{DESTINATION_PATH_FILE}"
assert result == [expected_uris]
+
+ @pytest.mark.parametrize("log_response", [True, False])
+ @mock.patch("airflow.providers.google.cloud.transfers.http_to_gcs.GCSHook")
+
@mock.patch("airflow.providers.google.cloud.transfers.http_to_gcs.HttpHook")
+ def test_execute_logs_response_only_when_requested(self, http_hook,
gcs_hook, caplog, log_response):
+ http_hook.return_value.run.return_value.text = "response body"
+ task = HttpToGCSOperator(
+ task_id=TASK_ID,
+ http_conn_id=HTTP_CONN_ID,
+ endpoint=ENDPOINT,
+ object_name=DESTINATION_PATH_FILE,
+ bucket_name=TEST_BUCKET,
+ gcp_conn_id=GCP_CONN_ID,
+ log_response=log_response,
+ )
+
+ with caplog.at_level(logging.INFO, logger=task.log.name):
+ task.execute(None)
+
+ assert ("response body" in caplog.text) is log_response
Review Comment:
Our testing standards say: *"Do not assert on raw log text (`caplog.text`
for example), these are legacy string matching APIs planned for removal."*
One caveat if you change it: the recommended `"event" in caplog` form will
not work for this provider, because `apache-airflow-providers-google` supports
Airflow `>=2.11.0` and its tests also run in the compat jobs, where `caplog`
falls back to the stock pytest fixture that has no `__contains__`. Two portable
options:
- `assert ("response body" in caplog.messages) is log_response` —
`.messages` exists on both the structlog capture and the stock fixture (the
amazon provider tests already use this form), or
- follow the http provider's own test for this same feature
(`providers/http/tests/unit/http/operators/test_http.py`,
`test_response_in_logs_after_failed_check`): patch `task.log.info` and assert
on `call_args_list` — then `import logging` and the `caplog` fixture are no
longer needed at all.
Not blocking the merge.
--
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]
