Add new base-path parameter to enhance the component yml from test workflow #3497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
zelinh
requested review from
dblock,
peterzhuamazon,
bbarani,
gaiksaya,
rishabh6788,
jordarlu,
prudhvigodithi,
Divyaasm and
tianleh
as code owners
zelinh
force-pushed
the
component_yml
branch
2 times, most recently
from
5bcd30c
to
b271043
Compare
Codecov Report
@@ Coverage Diff @@
## main #3497 +/- ##
==========================================
+ Coverage 91.16% 91.48% +0.31%
==========================================
Files 181 181
Lines 5342 5364 +22
==========================================
+ Hits 4870 4907 +37
+ Misses 472 457 -15
|
|
With this feature, the component yml file will be like with command If we don't specify the |
dblock
reviewed
May 11, 2023
src/test_workflow/test_args.py
Outdated
| @@ -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/ with trailing slash because of the property of urljoin | |||
There was a problem hiding this comment.
This will be very hard to debug, make it work without the trailing slash.
There was a problem hiding this comment.
Yeah this is found when I was looking into urljoin. https://stackoverflow.com/questions/10893374/python-confusions-with-urljoin
Let me see whether there is any alternative way of joining url or we could just join them with /.
zelinh
changed the title
Signed-off-by: Zelin Hao <[email protected]>
@dblock Sorry for a little confusion. I just updated the title of this PR to be more clear. This is basically enable a new parameter so that the component yml file generated from our test workflow will include realpath/URL of all the recorded logs. |
|
What is the default behavior if you do not provide a basepath? I assume the absolute paths of the files is recorded? |
@gaiksaya Yes, if not specified, real local path for those file would be displayed. #3497 (comment) |
|
Example of running integ test workflow without enabling |
|
New example of this with command: This job number is a real job number so that most of below URL are accessible. Only exception are the Change from |
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
gaiksaya
reviewed
May 18, 2023
| outcome = { | ||
| "test_type": self.test_type, | ||
| "test_run_id": self.test_run_id, | ||
| "run_id": self.test_run_id, |
There was a problem hiding this comment.
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.
Comment on lines
62
to
75
| 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 | ||
| } | ||
| ] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
Signed-off-by: Zelin Hao <[email protected]>
|
Updated with compatibility for integ tests on dashboards. The component yml file would include Command: |
|
The dashboard integration tests should also store the cypress artifacts as mentioned here https://github.com/opensearch-project/opensearch-build/blob/main/src/test_workflow/integ_test/integ_test_suite_opensearch_dashboards.py#L133-L137. These will fall under |
Signed-off-by: Zelin Hao <[email protected]>
@gaiksaya Updated with the new commits. Currently this component yml file would traverse through the results directory and listed out all those files. |
|
The new component yml file would include path/url to cypress folders (screenshots, reports, videos) Command: For OS, it remains the same |
gaiksaya
approved these changes
May 22, 2023
zelinh
added a commit
to zelinh/opensearch-build
that referenced
this pull request
May 23, 2023
…rkflow (opensearch-project#3497) Signed-off-by: Zelin Hao <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add new
base-pathparameter fortest_recorderto enhance the component yml generated from test workflow to display the location/URLs of local cluster logs or std text files.If this parameter is not specified, the component yml file will display the real local path of those files.
I also updated the
stdout&stderrpath by removing an extratest-resultsfolder.Change from
/usr/share/opensearch/opensearch-build/test-results/1234/integ-test/sql/with-security/test-results/stdout.txtto
/usr/share/opensearch/opensearch-build/test-results/1234/integ-test/sql/with-security/stdout.txtIssues Resolved
#3381
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.