Fix various problems in the pbench_results_push tests (#3378)

* Fix various problems in the pbench_results_push tests Fixes #3374 This is a continuation of #3348, implementing fixes to the pbench_results_push tests. The main one is to subsume some exception tests under the parametrized "normal" case and eliminate redundant tests.
distributed-system-analysis · Apr 12, 2023 · 9a57940 · 9a57940
1 parent 677ae18
commit 9a57940
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 139 deletions.
diff --git a/lib/pbench/test/unit/agent/task/test_copy_result_tb.py b/lib/pbench/test/unit/agent/task/test_copy_result_tb.py
@@ -99,7 +99,6 @@ def request_callback(request: requests.Request):
                 res = crt.copy_result_tb("token", access)
 
         assert res.status_code == HTTPStatus.CREATED
-        # If we got this far without an exception, then the test passes.
 
     @responses.activate
     @pytest.mark.parametrize(
@@ -143,7 +142,6 @@ def request_callback(request: requests.Request):
                 res = crt.copy_result_tb("token", access, metadata)
 
         assert res.status_code == HTTPStatus.CREATED
-        # If we got this far without an exception, then the test passes.
 
     @responses.activate
     def test_connection_error(self, monkeypatch, agent_logger):

diff --git a/lib/pbench/test/unit/agent/task/test_results_push.py b/lib/pbench/test/unit/agent/task/test_results_push.py
@@ -1,5 +1,4 @@
 from http import HTTPStatus
-import logging
 import os
 from typing import Dict, Optional, Union
 
@@ -26,15 +25,16 @@ class TestResultsPush:
 
     @staticmethod
     def add_http_mock_response(
-        status_code: HTTPStatus = None, message: Optional[Union[str, Dict]] = None
+        status_code: HTTPStatus = HTTPStatus.CREATED,
+        message: Optional[Union[str, Dict, Exception]] = None,
     ):
         parms = {}
         if status_code:
             parms["status"] = status_code
 
         if isinstance(message, dict):
             parms["json"] = message
-        elif isinstance(message, str):
+        elif isinstance(message, (str, Exception)):
             parms["body"] = message
 
         responses.add(
@@ -43,18 +43,6 @@ def add_http_mock_response(
             **parms,
         )
 
-    @staticmethod
-    def add_connectionerr_mock_response():
-        responses.add(
-            responses.PUT,
-            f"{TestResultsPush.URL}/upload/{os.path.basename(tarball)}",
-            body=requests.exceptions.ConnectionError(
-                "<urllib3.connection.HTTPConnection object at 0x1080854c0>: "
-                "Failed to establish a new connection: [Errno 8] "
-                "nodename nor servname provided, or not known"
-            ),
-        )
-
     @staticmethod
     @responses.activate
     def test_help():
@@ -101,7 +89,6 @@ def test_bad_arg():
     def test_meta_args():
         """Test operation when irregular arguments and options are specified"""
 
-        TestResultsPush.add_http_mock_response()
         runner = CliRunner(mix_stderr=False)
         result = runner.invoke(
             main,
@@ -162,27 +149,45 @@ def test_multiple_meta_args_single_option():
 
     @staticmethod
     @responses.activate
-    def test_multiple_meta_args():
-        """Test normal operation when all arguments and options are specified"""
+    def test_token_omitted():
+        """Test error when the token option is omitted"""
+
+        runner = CliRunner(mix_stderr=False)
+        result = runner.invoke(main, args=[tarball])
+        assert result.exit_code == 2, result.stderr
+        assert "Missing option '--token'" in str(result.stderr)
 
+    @staticmethod
+    @responses.activate
+    def test_token_envar(monkeypatch):
+        """Test normal operation with the token in an environment variable"""
+
+        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
         TestResultsPush.add_http_mock_response()
         runner = CliRunner(mix_stderr=False)
+        result = runner.invoke(main, args=[tarball])
+        assert result.exit_code == 0, result.stderr
+        assert result.stdout == ""
+
+    @staticmethod
+    @responses.activate
+    def test_access_error(monkeypatch):
+        """Test error in access value"""
+
+        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
+        runner = CliRunner(mix_stderr=False)
         result = runner.invoke(
             main,
             args=[
-                TestResultsPush.TOKN_SWITCH,
-                TestResultsPush.TOKN_TEXT,
-                TestResultsPush.ACCESS_SWITCH,
-                TestResultsPush.ACCESS_TEXT,
-                TestResultsPush.META_SWITCH,
-                TestResultsPush.META_TEXT_TEST,
-                TestResultsPush.META_SWITCH,
-                TestResultsPush.META_TEXT_FOO,
                 tarball,
+                TestResultsPush.ACCESS_SWITCH,
+                TestResultsPush.ACCESS_WRONG_TEXT,
             ],
         )
-        assert result.exit_code == 0, result.stderr
-        assert result.stdout == ""
+        assert result.exit_code == 2, result.stderr
+        assert "Error: Invalid value for '-a' / '--access': 'public/private'" in str(
+            result.stderr
+        )
 
     @staticmethod
     @responses.activate
@@ -202,6 +207,7 @@ def test_multiple_meta_args():
             (HTTPStatus.REQUEST_ENTITY_TOO_LARGE, "Request Entity Too Large", 1),
             (HTTPStatus.NOT_FOUND, {"message": "Not Found"}, 1),
             (HTTPStatus.NOT_FOUND, "Not Found", 1),
+            (None, requests.exceptions.ConnectionError("Oops"), 1),
         ),
     )
     def test_push_status(status_code, message, exit_code):
@@ -221,118 +227,21 @@ def test_push_status(status_code, message, exit_code):
                 tarball,
             ],
         )
+
         assert result.exit_code == exit_code, result.stderr
         assert result.stdout == ""
 
-        try:
-            err_msg = "" if not message else message["message"]
-        except TypeError:
+        if not message:
+            err_msg = ""
+        elif isinstance(message, dict):
+            err_msg = message["message"]
+        elif isinstance(message, str):
             err_msg = message
+        elif isinstance(message, Exception):
+            err_msg = str(message)
+        else:
+            assert False, "message must be dict, string, Exception or None"
 
-        if status_code >= HTTPStatus.BAD_REQUEST:
+        if status_code and status_code >= 400:
             err_msg = f"HTTP Error status: {status_code.value}, message: {err_msg}"
-        assert result.stderr.strip() == err_msg
-
-    @staticmethod
-    @responses.activate
-    def test_error_too_large_tarball():
-        """Test normal operation when all arguments and options are specified"""
-
-        TestResultsPush.add_http_mock_response(
-            status_code=HTTPStatus.REQUEST_ENTITY_TOO_LARGE,
-            message={"message": "Request Entity Too Large"},
-        )
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(
-            main,
-            args=[
-                TestResultsPush.TOKN_SWITCH,
-                TestResultsPush.TOKN_TEXT,
-                TestResultsPush.ACCESS_SWITCH,
-                TestResultsPush.ACCESS_TEXT,
-                TestResultsPush.META_SWITCH,
-                TestResultsPush.META_TEXT_TEST + "," + TestResultsPush.META_TEXT_FOO,
-                tarball,
-            ],
-        )
-        assert result.exit_code == 1, result.stderr
-        assert (
-            result.stderr
-            == "HTTP Error status: 413, message: Request Entity Too Large\n"
-        )
-
-    @staticmethod
-    @responses.activate
-    def test_token_omitted():
-        """Test error when the token option is omitted"""
-
-        TestResultsPush.add_http_mock_response()
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(main, args=[tarball])
-        assert result.exit_code == 2, result.stderr
-        assert "Missing option '--token'" in str(result.stderr)
-
-    @staticmethod
-    @responses.activate
-    def test_token_envar(monkeypatch, caplog):
-        """Test normal operation with the token in an environment variable"""
-
-        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
-        TestResultsPush.add_http_mock_response()
-        caplog.set_level(logging.DEBUG)
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(main, args=[tarball])
-        assert result.exit_code == 0, result.stderr
-        assert result.stdout == ""
-
-    @staticmethod
-    @responses.activate
-    def test_access_error(monkeypatch, caplog):
-        """Test error in access value"""
-
-        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
-        TestResultsPush.add_http_mock_response()
-        caplog.set_level(logging.DEBUG)
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(
-            main,
-            args=[
-                tarball,
-                TestResultsPush.ACCESS_SWITCH,
-                TestResultsPush.ACCESS_WRONG_TEXT,
-            ],
-        )
-        assert result.exit_code == 2, result.stderr
-        assert "Error: Invalid value for '-a' / '--access': 'public/private'" in str(
-            result.stderr
-        )
-
-    @staticmethod
-    @responses.activate
-    def test_connection_error(monkeypatch, caplog):
-        """Test handling of connection errors"""
-
-        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
-        TestResultsPush.add_connectionerr_mock_response()
-        caplog.set_level(logging.DEBUG)
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(main, args=[tarball])
-        assert result.exit_code == 1
-        assert str(result.stderr).startswith("Cannot connect to")
-
-    @staticmethod
-    @responses.activate
-    def test_http_error(monkeypatch, caplog):
-        """Test handling of 404 errors"""
-
-        monkeypatch.setenv("PBENCH_ACCESS_TOKEN", TestResultsPush.TOKN_TEXT)
-        TestResultsPush.add_http_mock_response(
-            HTTPStatus.NOT_FOUND, message={"message": "Not Found"}
-        )
-        caplog.set_level(logging.DEBUG)
-        runner = CliRunner(mix_stderr=False)
-        result = runner.invoke(main, args=[tarball])
-        assert result.exit_code == 1
-        assert (
-            "Not Found" in result.stderr
-        ), f"stderr: {result.stderr!r}; stdout: {result.stdout!r}"
+        assert err_msg in result.stderr