[GitHub] [airflow] o-nikolas commented on a diff in pull request #28710: Apply "unify bucket and key" before "provide bucket"

[GitBox <gi...@apache.org>]

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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org