From 7f5cc18ff19d04edc367d58c24f4a1d0a8e8a58d Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 26 Mar 2024 09:19:24 -0700 Subject: [PATCH 1/6] URL encode test result files Signed-off-by: Simeon Widdis --- src/test_workflow/test_recorder/test_recorder.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/test_workflow/test_recorder/test_recorder.py b/src/test_workflow/test_recorder/test_recorder.py index 4edad516df..e1bd646258 100644 --- a/src/test_workflow/test_recorder/test_recorder.py +++ b/src/test_workflow/test_recorder/test_recorder.py @@ -8,6 +8,7 @@ import logging import os import shutil +import urllib.parse from typing import Any import yaml @@ -73,7 +74,10 @@ def _generate_yml(self, test_result_data: TestResultData, output_path: str) -> s return os.path.realpath("%s.yml" % test_result_data.component_name) def _update_absolute_file_paths(self, files: list, base_path: str, relative_path: str) -> list: - return [os.path.join(base_path, relative_path, file) for file in files] + return [ + os.path.join(base_path, relative_path, urllib.parse.quote_plus(file)) + for file in files + ] # get a list of files within directory with relative paths. def _get_list_files(self, dir: str) -> list: From e8319068d955d0b59b1109cc0145044ea20b1a6c Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 26 Mar 2024 09:29:30 -0700 Subject: [PATCH 2/6] Add a test case for URL encoding Signed-off-by: Simeon Widdis --- .../test_recorder/test_test_recorder.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/tests_test_workflow/test_recorder/test_test_recorder.py b/tests/tests_test_workflow/test_recorder/test_test_recorder.py index 5b17f78b42..8ed4e7ffe3 100644 --- a/tests/tests_test_workflow/test_recorder/test_test_recorder.py +++ b/tests/tests_test_workflow/test_recorder/test_test_recorder.py @@ -99,6 +99,19 @@ def test_update_absolute_file_paths(self, mock_local_cluster_logs: Mock, mock_re file_path = test_recorder._update_absolute_file_paths(["file1", "file2"], "working-directory", "sub-directory") self.assertEqual(file_path, [os.path.join("working-directory", "sub-directory", "file1"), os.path.join("working-directory", "sub-directory", "file2")]) + @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") + @patch("test_workflow.test_recorder.test_recorder.RemoteClusterLogs") + @patch("test_workflow.test_recorder.test_recorder.LocalClusterLogs") + def test_update_absolute_file_paths(self, mock_local_cluster_logs: Mock, mock_remote_cluster_logs: Mock, mock_test_results_logs: Mock, *mock: Any) -> None: + test_recorder = TestRecorder( + "1234", + "integ-test", + "working-directory", + "https://ci.opensearch.org/ci/dbc/integ-test/" + ) + file_path = test_recorder._update_absolute_file_paths(["A long test case name"], "working-directory", "sub-directory") + self.assertEqual(file_path, [os.path.join("working-directory", "sub-directory", "A+long+test+case+name")]) + @patch("os.walk") @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") @patch("test_workflow.test_recorder.test_recorder.RemoteClusterLogs") From 68c46c1ed9e76b56851755967c0082734e4b8676 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 26 Mar 2024 10:39:12 -0700 Subject: [PATCH 3/6] Rename duplicated test method name Signed-off-by: Simeon Widdis --- tests/tests_test_workflow/test_recorder/test_test_recorder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tests_test_workflow/test_recorder/test_test_recorder.py b/tests/tests_test_workflow/test_recorder/test_test_recorder.py index 8ed4e7ffe3..10e002a0e6 100644 --- a/tests/tests_test_workflow/test_recorder/test_test_recorder.py +++ b/tests/tests_test_workflow/test_recorder/test_test_recorder.py @@ -102,7 +102,7 @@ def test_update_absolute_file_paths(self, mock_local_cluster_logs: Mock, mock_re @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") @patch("test_workflow.test_recorder.test_recorder.RemoteClusterLogs") @patch("test_workflow.test_recorder.test_recorder.LocalClusterLogs") - def test_update_absolute_file_paths(self, mock_local_cluster_logs: Mock, mock_remote_cluster_logs: Mock, mock_test_results_logs: Mock, *mock: Any) -> None: + def test_update_absolute_file_paths_escaping(self, mock_local_cluster_logs: Mock, mock_remote_cluster_logs: Mock, mock_test_results_logs: Mock, *mock: Any) -> None: test_recorder = TestRecorder( "1234", "integ-test", From ddf9bed3ecaa73ecaf2dcdb247f242b72a2f6c5b Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 27 Mar 2024 15:51:39 -0700 Subject: [PATCH 4/6] Only escape https links Signed-off-by: Simeon Widdis --- src/test_workflow/test_recorder/test_recorder.py | 6 +++++- .../test_recorder/test_test_recorder.py | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/test_workflow/test_recorder/test_recorder.py b/src/test_workflow/test_recorder/test_recorder.py index e1bd646258..f208d8fe1d 100644 --- a/src/test_workflow/test_recorder/test_recorder.py +++ b/src/test_workflow/test_recorder/test_recorder.py @@ -75,7 +75,11 @@ def _generate_yml(self, test_result_data: TestResultData, output_path: str) -> s def _update_absolute_file_paths(self, files: list, base_path: str, relative_path: str) -> list: return [ - os.path.join(base_path, relative_path, urllib.parse.quote_plus(file)) + os.path.join( + base_path, + relative_path, + urllib.parse.quote_plus(file) if base_path.startswith("https://") else file + ) for file in files ] diff --git a/tests/tests_test_workflow/test_recorder/test_test_recorder.py b/tests/tests_test_workflow/test_recorder/test_test_recorder.py index 10e002a0e6..273f3c9ea6 100644 --- a/tests/tests_test_workflow/test_recorder/test_test_recorder.py +++ b/tests/tests_test_workflow/test_recorder/test_test_recorder.py @@ -109,8 +109,8 @@ def test_update_absolute_file_paths_escaping(self, mock_local_cluster_logs: Mock "working-directory", "https://ci.opensearch.org/ci/dbc/integ-test/" ) - file_path = test_recorder._update_absolute_file_paths(["A long test case name"], "working-directory", "sub-directory") - self.assertEqual(file_path, [os.path.join("working-directory", "sub-directory", "A+long+test+case+name")]) + file_path = test_recorder._update_absolute_file_paths(["A long test case name"], "https://working-directory", "sub-directory") + self.assertEqual(file_path, [os.path.join("https://working-directory", "sub-directory", "A+long+test+case+name")]) @patch("os.walk") @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") @@ -123,9 +123,9 @@ def test_get_list_files(self, mock_local_cluster_logs: Mock, mock_remote_cluster "working-directory", "https://ci.opensearch.org/ci/dbc/integ-test/" ) - mock_os_walk.return_value = [("mock_dir", (), ("file1", "file2"))] + mock_os_walk.return_value = [("mock_dir", (), ("file1", "file2 with spaces"))] list_files = test_recorder._get_list_files("mock_dir") - self.assertEqual(list_files, ["file1", "file2"]) + self.assertEqual(list_files, ["file1", "file2 with spaces"]) @patch("builtins.open", new_callable=mock_open) def test_generate_std_files(self, mock_open: Mock, *mock: Any) -> None: From 025a609dad62de3cfb4de9d191a745b4ff9418dc Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 28 Mar 2024 20:27:04 -0700 Subject: [PATCH 5/6] Update path joining to use forward slashes in https branch Signed-off-by: Simeon Widdis --- .../test_recorder/test_recorder.py | 22 ++++++++----------- .../test_recorder/test_test_recorder.py | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/test_workflow/test_recorder/test_recorder.py b/src/test_workflow/test_recorder/test_recorder.py index f208d8fe1d..eca0f2c386 100644 --- a/src/test_workflow/test_recorder/test_recorder.py +++ b/src/test_workflow/test_recorder/test_recorder.py @@ -50,9 +50,9 @@ def _create_base_folder_structure(self, component_name: str, component_test_conf return os.path.realpath(dest_directory) def _generate_std_files(self, stdout: str, stderr: str, output_path: str) -> None: - with open(os.path.join(output_path, "stdout.txt"), "w", encoding='utf-8') as stdout_file: + with open(os.path.join(output_path, "stdout.txt"), "w", encoding="utf-8") as stdout_file: stdout_file.write(stdout) - with open(os.path.join(output_path, "stderr.txt"), "w", encoding='utf-8') as stderr_file: + with open(os.path.join(output_path, "stderr.txt"), "w", encoding="utf-8") as stderr_file: stderr_file.write(stderr) def _generate_yml(self, test_result_data: TestResultData, output_path: str) -> str: @@ -67,21 +67,17 @@ def _generate_yml(self, test_result_data: TestResultData, output_path: str) -> s "component_name": test_result_data.component_name, "test_config": test_result_data.component_test_config, "test_result": "PASS" if (test_result_data.exit_code == 0) else "FAIL", - "test_result_files": test_result_file + "test_result_files": test_result_file, } - with open(os.path.join(output_path, "%s.yml" % test_result_data.component_name), "w", encoding='utf-8') as file: + with open(os.path.join(output_path, "%s.yml" % test_result_data.component_name), "w", encoding="utf-8") as file: yaml.dump(outcome, file) return os.path.realpath("%s.yml" % test_result_data.component_name) def _update_absolute_file_paths(self, files: list, base_path: str, relative_path: str) -> list: - return [ - os.path.join( - base_path, - relative_path, - urllib.parse.quote_plus(file) if base_path.startswith("https://") else file - ) - for file in files - ] + if base_path.startswith("https://"): + return [f"{base_path}/{relative_path}/{urllib.parse.quote_plus(file)}" for file in files] + else: + return [os.path.join(base_path, relative_path, file) for file in files] # get a list of files within directory with relative paths. def _get_list_files(self, dir: str) -> list: @@ -141,7 +137,7 @@ def save_test_result_data(self, test_result_data: TestResultData) -> None: class TestResultsLogs(LogRecorder): - __test__ = False # type:ignore + __test__ = False # type:ignore parent_class: TestRecorder def __init__(self, parent_class: TestRecorder) -> None: diff --git a/tests/tests_test_workflow/test_recorder/test_test_recorder.py b/tests/tests_test_workflow/test_recorder/test_test_recorder.py index 273f3c9ea6..422500e48b 100644 --- a/tests/tests_test_workflow/test_recorder/test_test_recorder.py +++ b/tests/tests_test_workflow/test_recorder/test_test_recorder.py @@ -110,7 +110,7 @@ def test_update_absolute_file_paths_escaping(self, mock_local_cluster_logs: Mock "https://ci.opensearch.org/ci/dbc/integ-test/" ) file_path = test_recorder._update_absolute_file_paths(["A long test case name"], "https://working-directory", "sub-directory") - self.assertEqual(file_path, [os.path.join("https://working-directory", "sub-directory", "A+long+test+case+name")]) + self.assertEqual(file_path, ["https://working-directory/sub-directory/A+long+test+case+name"]) @patch("os.walk") @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") From 4bc14e3bbbe41c649b67fb705f06de89a1bdfa15 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 28 Mar 2024 20:31:30 -0700 Subject: [PATCH 6/6] Move 'with spaces' file to correct test Signed-off-by: Simeon Widdis --- .../test_recorder/test_test_recorder.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/tests_test_workflow/test_recorder/test_test_recorder.py b/tests/tests_test_workflow/test_recorder/test_test_recorder.py index 422500e48b..a4a1751585 100644 --- a/tests/tests_test_workflow/test_recorder/test_test_recorder.py +++ b/tests/tests_test_workflow/test_recorder/test_test_recorder.py @@ -96,8 +96,15 @@ def test_update_absolute_file_paths(self, mock_local_cluster_logs: Mock, mock_re "working-directory", "https://ci.opensearch.org/ci/dbc/integ-test/" ) - file_path = test_recorder._update_absolute_file_paths(["file1", "file2"], "working-directory", "sub-directory") - self.assertEqual(file_path, [os.path.join("working-directory", "sub-directory", "file1"), os.path.join("working-directory", "sub-directory", "file2")]) + file_path = test_recorder._update_absolute_file_paths( + ["file1", "file2 with spaces"], + "working-directory", + "sub-directory" + ) + self.assertEqual(file_path, [ + os.path.join("working-directory", "sub-directory", "file1"), + os.path.join("working-directory", "sub-directory", "file2 with spaces") + ]) @patch("test_workflow.test_recorder.test_recorder.TestResultsLogs") @patch("test_workflow.test_recorder.test_recorder.RemoteClusterLogs") @@ -123,9 +130,9 @@ def test_get_list_files(self, mock_local_cluster_logs: Mock, mock_remote_cluster "working-directory", "https://ci.opensearch.org/ci/dbc/integ-test/" ) - mock_os_walk.return_value = [("mock_dir", (), ("file1", "file2 with spaces"))] + mock_os_walk.return_value = [("mock_dir", (), ("file1", "file2"))] list_files = test_recorder._get_list_files("mock_dir") - self.assertEqual(list_files, ["file1", "file2 with spaces"]) + self.assertEqual(list_files, ["file1", "file2"]) @patch("builtins.open", new_callable=mock_open) def test_generate_std_files(self, mock_open: Mock, *mock: Any) -> None: