o-nikolas commented on code in PR #28710:
URL: https://github.com/apache/airflow/pull/28710#discussion_r1061955523
##########
tests/providers/amazon/aws/hooks/test_s3.py:
##########
@@ -884,66 +883,75 @@ def do_something(self, bucket_name=None, key=None):
else:
- class MyHook(S3Hook):
- @provide_bucket_name
- @unify_bucket_name_and_key
- def do_something(self, bucket_name=None, key=None):
- return bucket_name, key
+ with caplog.at_level("WARNING"):
+ class MyHook(S3Hook):
+ @provide_bucket_name
+ @unify_bucket_name_and_key
+ def do_something(self, bucket_name=None, key=None):
+ return bucket_name, key
+
+ assert caplog.records[0].message == "`unify_bucket_name_and_key`
should wrap `provide_bucket_name`."
hook = MyHook()
- if expected == "__fail__":
- with pytest.raises(Exception, match='Please provide a bucket name
using a valid format: "key.txt"'):
- hook.do_something(**kwargs)
- else:
- assert list(hook.do_something(**kwargs)) == expected
+ assert list(hook.do_something(**kwargs)) == expected
@pytest.mark.parametrize(
- "expected",
+ "has_conn, has_bucket, key_kind, expected",
[
# full key
# no conn - no bucket - full key
- param(["key_bucket", "key.txt"], id="no_conn-no_bucket-full_key"),
+ ("no_conn", "no_bucket", "full_key", ["key_bucket", "key.txt"]),
# no conn - with bucket - full key
- param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="no_conn-with_bucket-full_key"),
+ ("no_conn", "with_bucket", "full_key", ["kwargs_bucket",
"s3://key_bucket/key.txt"]),
# with conn - no bucket - full key
- param(["conn_bucket", "s3://key_bucket/key.txt"],
id="with_conn-no_bucket-full_key"),
+ ("with_conn", "no_bucket", "full_key", ["key_bucket", "key.txt"]),
# with conn - with bucket - full key
- param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="with_conn-with_bucket-full_key"),
+ ("with_conn", "with_bucket", "full_key", ["kwargs_bucket",
"s3://key_bucket/key.txt"]),
# rel key
# no conn - no bucket - rel key
- param("__fail__", id="no_conn-no_bucket-rel_key"),
+ ("no_conn", "no_bucket", "rel_key", [None, "key.txt"]),
# no conn - with bucket - rel key
- param(["kwargs_bucket", "key.txt"], id="no_conn-with_bucket-rel_key"),
+ ("no_conn", "with_bucket", "rel_key", ["kwargs_bucket", "key.txt"]),
# with conn - no bucket - rel key
- param(["conn_bucket", "key.txt"], id="with_conn-no_bucket-rel_key"),
+ ("with_conn", "no_bucket", "rel_key", ["conn_bucket", "key.txt"]),
# with conn - with bucket - rel key
- param(["kwargs_bucket", "key.txt"],
id="with_conn-with_bucket-rel_key"),
+ ("with_conn", "with_bucket", "rel_key", ["kwargs_bucket", "key.txt"]),
],
)
@patch("airflow.hooks.base.BaseHook.get_connection")
-def test_s3_head_object_decorated_behavior(mock_conn, request, expected):
- tokens = request.node.callspec.id.split("-")
- assert len(tokens) == 3
- if "with_conn" in tokens:
+def test_s3_head_object_decorated_behavior(mock_conn, has_conn, has_bucket,
key_kind, expected):
+ if has_conn == "with_conn":
Review Comment:
Thanks for making these changes @dstandish! It's very much appreciated
:pray: and is indeed what I was describing before. The only other improvement
is that you can actually just make many of the params here simply booleans
instead of strings. This would allow you to get rid of the string comparisons
in the conditionals of the test body.
But of course feel free to do this or not :)
P.S. I tried, but unfortunately cannot, add a full suggested change for the
above because the snippet includes deleted lines 927/928 and the GitHub UI does
not allow that, and I didn't want to include a partial one so as to avoid
confusion.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
