Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new base-path parameter to enhance the component yml from test workflow #3497

Merged
merged 8 commits into from
May 22, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/test_workflow/integ_test/integ_test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def __init__(self, args: TestArgs, test_manifest: TestManifest, components: Comp

self.tests_dir = os.path.join(os.getcwd(), "test-results")
os.makedirs(self.tests_dir, exist_ok=True)
self.test_recorder = TestRecorder(self.args.test_run_id, "integ-test", self.tests_dir)
self.test_recorder = TestRecorder(self.args.test_run_id, "integ-test", self.tests_dir, args.base_path)

def run(self) -> TestSuiteResults:
with TemporaryDirectory(keep=self.args.keep, chdir=True) as work_dir:
Expand All @@ -45,6 +45,7 @@ def run(self) -> TestSuiteResults:
if test_config.integ_test:
test_suite = self.__create_test_suite__(component, test_config, work_dir.path)
test_results = test_suite.execute_tests()
[self.test_recorder.test_results_logs.save_test_result_data(result_data) for result_data in test_suite.result_data]
all_results.append(component.name, test_results)
else:
logging.info(f"Skipping integ-tests for {component.name}, as it is currently not supported")
Expand Down
6 changes: 6 additions & 0 deletions src/test_workflow/integ_test/integ_test_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class IntegTestSuite(abc.ABC):
build_manifest: BuildManifest
save_logs: LogRecorder
additional_cluster_config: dict
test_result_data: list

"""
Kicks off integration tests for a component based on test configurations provided in
Expand Down Expand Up @@ -55,6 +56,7 @@ def __init__(

self.save_logs = test_recorder.test_results_logs
self.additional_cluster_config = None
self.test_result_data = []

@abc.abstractmethod
def execute_tests(self) -> TestComponentResults:
Expand All @@ -80,6 +82,10 @@ def pretty_print_message(self, message: str) -> None:
def test_artifact_files(self) -> Dict[str, str]:
pass

@property
def result_data(self) -> list:
return self.test_result_data


class InvalidTestConfigError(Exception):
pass
17 changes: 9 additions & 8 deletions src/test_workflow/integ_test/integ_test_suite_opensearch.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,16 @@ def custom_node_endpoint_encoder(node_endpoint: NodeEndpoint) -> dict:
self.repo_work_dir = os.path.join(
self.repo.dir, self.test_config.working_directory) if self.test_config.working_directory is not None else self.repo.dir
(status, stdout, stderr) = execute(cmd, self.repo_work_dir, True, False)
test_result_data = TestResultData(
self.component.name,
test_config,
status,
stdout,
stderr,
self.test_artifact_files
self.test_result_data.append(
TestResultData(
self.component.name,
test_config,
status,
stdout,
stderr,
self.test_artifact_files
)
)
self.save_logs.save_test_result_data(test_result_data)
if stderr:
logging.info("Stderr reported for component: " + self.component.name)
logging.info(stderr)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,16 @@ def custom_node_endpoint_encoder(node_endpoint: NodeEndpoint) -> dict:
self.repo_work_dir = os.path.join(
self.repo.dir, self.test_config.working_directory) if self.test_config.working_directory is not None else self.repo.dir
(status, stdout, stderr) = execute(cmd, self.repo_work_dir, True, False)
test_result_data = TestResultData(
self.component.name,
test_config,
status,
stdout,
stderr,
self.test_artifact_files
self.test_result_data.append(
TestResultData(
self.component.name,
test_config,
status,
stdout,
stderr,
self.test_artifact_files
)
)
self.save_logs.save_test_result_data(test_result_data)
if stderr:
logging.info("Stderr reported for component: " + self.component.name)
logging.info(stderr)
Expand Down
3 changes: 3 additions & 0 deletions src/test_workflow/test_args.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ def __init__(self) -> None:
parser = argparse.ArgumentParser(description="Test an OpenSearch Bundle")
parser.add_argument("test_manifest_path", type=TestArgsPathValidator.validate, help="Specify a test manifest path.")
parser.add_argument("-p", "--paths", nargs='*', action=TestKwargs, default={}, help="Specify paths for OpenSearch and OpenSearch Dashboards.")
# e.g. --base-path https://ci.opensearch.org/ci/dbc/integ-test/2.7.0/7771/linux/x64/tar/
parser.add_argument("--base-path", type=str, default="", help="Specify base paths for the integration test logs.")
parser.add_argument("--test-run-id", type=int, help="The unique execution id for the test")
parser.add_argument("--component", type=str, dest="components", nargs='*', help="Test a specific component or components instead of the entire distribution.")
parser.add_argument("--keep", dest="keep", action="store_true", help="Do not delete the working temporary directory.")
Expand All @@ -41,6 +43,7 @@ def __init__(self) -> None:
self.logging_level = args.logging_level
self.test_manifest_path = args.test_manifest_path
self.paths = args.paths
self.base_path = args.base_path


TestArgs.__test__ = False # type:ignore
47 changes: 41 additions & 6 deletions src/test_workflow/test_recorder/test_recorder.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class TestRecorder:
remote_cluster_logs: Any
test_results_logs: Any

def __init__(self, test_run_id: str, test_type: str, tests_dir: str) -> None:
def __init__(self, test_run_id: str, test_type: str, tests_dir: str, base_path: str = None) -> None:
self.test_run_id = test_run_id
self.test_type = test_type
self.location = os.path.join(tests_dir, str(self.test_run_id), self.test_type)
Expand All @@ -33,6 +33,14 @@ def __init__(self, test_run_id: str, test_type: str, tests_dir: str) -> None:
self.local_cluster_logs = LocalClusterLogs(self)
self.remote_cluster_logs = RemoteClusterLogs(self)
self.test_results_logs = TestResultsLogs(self)
self.base_path = base_path

def _get_file_path(self, base_path: str, component_name: str, component_test_config: str) -> str:
if base_path.startswith("https://"):
file_path = "/".join([base_path.strip("/"), "test-results", str(self.test_run_id), self.test_type, component_name, component_test_config])
else:
file_path = self._create_base_folder_structure(component_name, component_test_config)
return file_path

def _create_base_folder_structure(self, component_name: str, component_test_config: str) -> str:
dest_directory = os.path.join(self.location, component_name, component_test_config)
Expand All @@ -46,17 +54,45 @@ def _generate_std_files(self, stdout: str, stderr: str, output_path: str) -> Non
stderr_file.write(stderr)

def _generate_yml(self, test_result_data: TestResultData, output_path: str) -> str:
base_file_path = self._get_file_path(self.base_path, test_result_data.component_name, test_result_data.component_test_config)

components_files = self._get_list_files(output_path)
components_files = self._update_absolute_file_paths(components_files, base_file_path, "")

local_cluster_logs_path = os.path.join(output_path, "local-cluster-logs")
local_cluster_logs = self._get_list_files(local_cluster_logs_path)
local_cluster_logs = self._update_absolute_file_paths(local_cluster_logs, base_file_path, "local-cluster-logs")

opensearch_service_logs_path = os.path.join(local_cluster_logs_path, "opensearch-service-logs")
opensearch_service_logs = self._get_list_files(opensearch_service_logs_path)
opensearch_service_logs = self._update_absolute_file_paths(opensearch_service_logs, base_file_path, os.path.join("local-cluster-logs", "opensearch-service-logs"))

test_result_file = components_files + [
{
"local-cluster-logs":
local_cluster_logs + opensearch_service_logs
}
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want this to be generic so that any log files can be written under here. Did you check how this behavior affects dashboards integ-test? Since this just seems to be related to opensearch logs and test_recorder is generic to all tests plus OS and OSD. Should this be part of LocalClusterLogs class?
https://github.com/opensearch-project/opensearch-build/pull/3497/files#diff-d8e45f5fe0a4500dc4bb059941f629bcc07c6cff714a4a6ee1711dc1fdc7c08aR104

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaiksaya
For integ test on dashboards, the behavior here are the same. The component yml file (functionalTestDashboards.yml) would list out the stdout&stderr for component and local cluster logs since it doesn't contain opensearch_service. The only things it wouldn't include are those cypress folders (e.g. cypress-screenshots/cypress-videos/cypress-report). I don't think it necessary to keep track of them in the scope of this PR as they are relatively too much to list them all. We could anyway track those videos/reports from the OSD team website I believe.

I don't think we should have this be part of the LocalClusterLogs as that class is mainly for recording and generating those files. Here we basically just list them out in the component yml file.


outcome = {
"test_type": self.test_type,
"test_run_id": self.test_run_id,
"run_id": self.test_run_id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the rename?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want this key to be written above the test_result_files when generating the yaml file but not strong opinion to change it. Open to change it back.

"component_name": test_result_data.component_name,
"test_config": test_result_data.component_test_config,
"exit_code": test_result_data.exit_code,
"test_result": "PASS" if (test_result_data.exit_code == 0) else "FAIL",
"test_result_files": test_result_file
}
with open(os.path.join(output_path, "%s.yml" % test_result_data.component_name), "w") 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, file) for file in files]

# get a list of files within directory without folders.
def _get_list_files(self, dir: str) -> list:
return [f for f in os.listdir(dir) if os.path.isfile(dir + '/' + f)]

def _copy_log_files(self, log_files: dict, dest_directory: str) -> None:
if log_files:
for log_dest_dir_name, source_log_dir in log_files.items():
Expand Down Expand Up @@ -106,15 +142,14 @@ def save_test_result_data(self, test_result_data: TestResultData) -> None:


class TestResultsLogs(LogRecorder):
__test__ = False # type:ignore
parent_class: TestRecorder

def __init__(self, parent_class: TestRecorder) -> None:
self.parent_class = parent_class

def save_test_result_data(self, test_result_data: TestResultData) -> None:
base = self.parent_class._create_base_folder_structure(test_result_data.component_name, test_result_data.component_test_config)
dest_directory = os.path.join(base, "test-results")
os.makedirs(dest_directory, exist_ok=False)
dest_directory = self.parent_class._create_base_folder_structure(test_result_data.component_name, test_result_data.component_test_config)
logging.info(f"Recording component test results for {test_result_data.component_name} at " f"{os.path.realpath(dest_directory)}")
self.parent_class._generate_std_files(test_result_data.stdout, test_result_data.stderr, dest_directory)
self.parent_class._copy_log_files(test_result_data.log_files, dest_directory)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,18 @@ def test_with_integ_test(self, mock_temp: Mock, mock_test_recorder: Mock, mock_s
mock_test_recorder_object = MagicMock()
mock_test_recorder.return_value = mock_test_recorder_object

mock_suite_object.result_data.__iter__.return_value = [MagicMock(), MagicMock()]

runner = IntegTestRunnerOpenSearch(self.args, self.test_manifest)

# call the test target
results = runner.run()

self.assertEqual(results["sql"], mock_test_results)

mock_suite_object.result_data.__iter__.assert_called()
mock_test_recorder_object.test_results_logs.save_test_result_data.assert_called()

mock_suite.assert_called_once_with(
mock_properties_object.dependency_installer,
mock_component,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,18 @@ def test_with_integ_test(self, mock_temp: Mock, mock_test_recorder: Mock, mock_s
mock_test_recorder_object = MagicMock()
mock_test_recorder.return_value = mock_test_recorder_object

mock_suite_object.result_data.__iter__.return_value = [MagicMock(), MagicMock()]

runner = IntegTestRunnerOpenSearchDashboards(self.args, self.test_manifest)

# call the test target
results = runner.run()

self.assertEqual(results["sql"], mock_test_results)

mock_suite_object.result_data.__iter__.assert_called()
mock_test_recorder_object.test_results_logs.save_test_result_data.assert_called()

mock_suite.assert_called_once_with(
mock_properties_dependency_object.dependency_installer,
mock_properties_object.dependency_installer,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def test_execute_integtest_sh(self, mock_execute: Mock, mock_git: Mock, mock_tes
"cypress-report": os.path.join("dir", "test_working_directory", "cypress", "results")
}
)
self.save_logs.save_test_result_data.assert_called_once_with(mock_test_result_data_object)
assert(mock_test_result_data.return_value in suite.result_data)

@patch("os.path.exists")
@patch("test_workflow.integ_test.integ_test_suite_opensearch_dashboards.TestResultData")
Expand Down

This file was deleted.

Loading