-
Notifications
You must be signed in to change notification settings - Fork 272
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
Update integ test runner to record each configuration separately #4438
Conversation
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4438 +/- ##
==========================================
+ Coverage 91.35% 92.19% +0.84%
==========================================
Files 190 192 +2
Lines 6197 6290 +93
==========================================
+ Hits 5661 5799 +138
+ Misses 536 491 -45 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
7551b3b
to
61b7422
Compare
@@ -35,9 +35,10 @@ def setUp(self) -> None: | |||
self.work_dir = Path("test_dir") | |||
|
|||
@patch("os.path.exists", return_value=True) | |||
@patch("test_workflow.integ_test.integ_test_suite_opensearch.IntegTestSuiteOpenSearch.multi_execute_integtest_sh") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this and below tests updated is the behavior of python mock tests.
We were mocking IntegTestSuiteOpenSearch.multi_execute_integtest_sh
here so it will apply to all tests intended to run IntegTestSuiteOpenSearch.multi_execute_integtest_sh
within this class. Patching this to enforce this mock only applied to this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zelinh ,
Generally great PR to help resolve the override videos/screenshots issues.
Just a nit on the variable name. Thanks!
stderr, | ||
self.test_artifact_files | ||
) | ||
test_result_data = TestResultData( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: name this local var as test_result_data_obj
or test_result_data_local
to distinguish from self.test_result_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the variable name. Please help take a look again. Thanks!
stderr, | ||
self.test_artifact_files | ||
) | ||
test_result_data = TestResultData( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: name this local var as test_result_data_obj
or test_result_data_local
to distinguish from self.test_result_data
.
Signed-off-by: Zelin Hao <zelinhao@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, @zelinh !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, @zelinh !
Description
Change test runner to record test results each time it finished testing with one configuration.
Generate the component yaml file after both
with-security
andwithout-security
tests completed because the local cluster logs will only be generated and recorded after process killed.Issues Resolved
#4435
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.