dstandish commented on code in PR #28707:
URL: https://github.com/apache/airflow/pull/28707#discussion_r1061050121
##########
tests/providers/amazon/aws/hooks/test_s3.py:
##########
@@ -825,3 +826,122 @@ def test_delete_bucket_tagging_with_no_tags(self):
with pytest.raises(ClientError, match=r".*NoSuchTagSet.*"):
hook.get_bucket_tagging(bucket_name="new_bucket")
+
+
+@patch("airflow.hooks.base.BaseHook.get_connection")
[email protected](
+ "expected",
+ [
+ # full key
+ # no conn - no bucket - full key
+ param(["key_bucket", "key.txt"],
id="unify-no_conn-no_bucket-full_key"),
+ param(["key_bucket", "key.txt"],
id="provide-no_conn-no_bucket-full_key"),
+ # no conn - with bucket - full key
+ param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="unify-no_conn-with_bucket-full_key"),
+ param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="provide-no_conn-with_bucket-full_key"),
+ # with conn - no bucket - full key
+ param(["conn_bucket", "s3://key_bucket/key.txt"],
id="provide-with_conn-no_bucket-full_key"),
+ param(["key_bucket", "key.txt"],
id="unify-with_conn-no_bucket-full_key"),
+ # with conn - with bucket - full key
+ param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="provide-with_conn-with_bucket-full_key"),
+ param(["kwargs_bucket", "s3://key_bucket/key.txt"],
id="unify-with_conn-with_bucket-full_key"),
+ # rel key
+ # no conn - no bucket - rel key
+ param("__fail__", id="unify-no_conn-no_bucket-rel_key"),
+ param("__fail__", id="provide-no_conn-no_bucket-rel_key"),
+ # no conn - with bucket - rel key
+ param(["kwargs_bucket", "key.txt"],
id="unify-no_conn-with_bucket-rel_key"),
+ param(["kwargs_bucket", "key.txt"],
id="provide-no_conn-with_bucket-rel_key"),
+ # with conn - no bucket - rel key
+ param(["conn_bucket", "key.txt"],
id="provide-with_conn-no_bucket-rel_key"),
+ param("__fail__", id="unify-with_conn-no_bucket-rel_key"),
+ # with conn - with bucket - rel key
+ param(["kwargs_bucket", "key.txt"],
id="provide-with_conn-with_bucket-rel_key"),
+ param(["kwargs_bucket", "key.txt"],
id="unify-with_conn-with_bucket-rel_key"),
+ ],
+)
+def test_dec_no_get_connection_call(mock_base, expected, request):
+ tokens = request.node.callspec.id.split("-")
+ assert len(tokens) == 4
+ if "with_conn" in tokens:
+ c = Connection(schema="conn_bucket")
+ else:
+ c = Connection(schema=None)
+ key = "key.txt" if "rel_key" in tokens else "s3://key_bucket/key.txt"
Review Comment:
the problem with your suggestion is what goes in the `[...]`
if you try it, you will see. you are welcome to try it, and if you like
where you get, you can add it as a suggestion or make a followup PR.
if you are not williing to do it yourself, then let's consider this resolved
please.
i understand how parametrized is meant to be used. i have written many such
parametrized tests, and never have i done this before. but i maintain that
here it's a pragmatic solution.
i am just doing my part here to make airflow better by fixing problems that
i happen upon. this is a tangent for me. i have more important work to get
back to. and i believe that this test, which doesn't use parametrized in the
platonic ideal manner, is nonetheless adequate.
and to be clear i do not mean to disparage the existing code with that
comment -- i just mean to put things in context. i am fixing and improving
things here. let's not let perfect be the enemy of good, and let's not lose
focus on the forest and get stuck on a tree, particularly when that tree is
test code. test code has different standards. in some ways tests are meant to
be a little brittle.
i don't mind suggestions, i appreciate them. but i don't appreciate it
when, if a suggestion is rejected in good faith and for legitimate reasons,
"no" is not taken for an answer. as reviewers we must separate "this is
genuinely problematic" from "i would have done it differently". and if it's
"there's nothing wrong with this but i would have done it differently", then we
should just comment and let author decide and accept their decision. this is a
very difficult thing to balance, no doubt, and i struggle it personally in my
reviewing. so no judgment here just letting you know where i'm coming from.
--
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]
