Added Test Publisher #464
Added Test Publisher #464
Conversation
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
| import os | ||
| import sys | ||
|
|
||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../src")) |
There was a problem hiding this comment.
Is there any better way to do this? Require this to import aws module below
There was a problem hiding this comment.
This should not be required at all when run from the test.sh driver because it introduces a stable path. That's why we have those things.
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
| import os | ||
| import sys | ||
|
|
||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../src")) |
There was a problem hiding this comment.
This should not be required at all when run from the test.sh driver because it introduces a stable path. That's why we have those things.
| from aws.s3_bucket import S3Bucket | ||
|
|
||
|
|
||
| class TestPublisher: |
There was a problem hiding this comment.
I suggest renaming this to TestResults, since it wraps test results, and rename publish_test_results_to_s3 to to_s3, similar to other to_file or to_dict that we have on other classes.
I would add TestRecorder.results that returns an instance of test results.
Remove s3_bucket from the constructor since it never consumes S3 results, and make it a parameter in to_s3, so TestResults().to_s3(bucket).
There was a problem hiding this comment.
I will change the name in the proposal as well #207 (comment)
There was a problem hiding this comment.
TestPublisher feels right to me as this class really just publishes the results while wrapping them up in the right directory structure. But I would leave it to dB who is our master of renaming :).
There was a problem hiding this comment.
@dblock any thoughts on this?
There was a problem hiding this comment.
No strong feelings. You're choosing between a verb and a noun. In OO I prefer nouns.
owaiskazi19
force-pushed
the
test-publisher
branch
from
ed3932a to
5d06517
Compare
Signed-off-by: Owais Kazi <[email protected]>
owaiskazi19
force-pushed
the
test-publisher
branch
from
5d06517 to
91a16bb
Compare
| from test_workflow.test_results import TestResults | ||
|
|
||
|
|
||
| class TestTestResults(unittest.TestCase): |
| from aws.s3_bucket import S3Bucket | ||
|
|
||
|
|
||
| class TestPublisher: |
There was a problem hiding this comment.
TestPublisher feels right to me as this class really just publishes the results while wrapping them up in the right directory structure. But I would leave it to dB who is our master of renaming :).
| Publishes tests results to S3 pulling information from {self.test_recorder} | ||
| And cleans up all local storage after publishing ({self.test_recorder}.clean_up()) | ||
| """ | ||
| s3_bucket = S3Bucket(bucket, '<role-arn>', 'test-publisher-session') |
There was a problem hiding this comment.
I think we already have a role defined. @gaiksaya could help and chime in with the role we have already created in Jenkins?
There was a problem hiding this comment.
Yes. Asked @owaiskazi19 to remove this params except the s3bucket. They are not required.
| def to_s3(self, bucket): | ||
| """ | ||
| Publishes tests results to S3 pulling information from {self.test_recorder} | ||
| And cleans up all local storage after publishing ({self.test_recorder}.clean_up()) |
There was a problem hiding this comment.
Fix this comment as TestRecorder cleans it up by itself and TestPublisher doesn't have to be worried about it.
There was a problem hiding this comment.
TestRecorder is not cleaning up. I removed the clean_up block but it is creating the temporary directory (when the user doesnot provide the location specifically) which I assume will be cleaned up eventually by the system?
There was a problem hiding this comment.
Responded on the other PR so that its clear: https://github.com/opensearch-project/opensearch-build/pull/467/files#r708746645
I'll update the comment on #207
Signed-off-by: Owais Kazi <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
|
Can open this PR once #467 gets merged. |
| self.bundle_manifest = bundle_manifest | ||
| self.test_recorder = test_recorder | ||
|
|
||
| def to_s3(self): |
There was a problem hiding this comment.
we also need a to_local method to publish logs locally for devs
|
#467 was merged, what's next for this? |
|
@owaiskazi19 it looks like #587 has been merged, did you want to pick this back up? |
Hey @peternied! As discussed, @gaiksaya will be working on the remaining chunks of TestPublisher. #341. |
|
Let's close this until @gaiksaya re-raises a working PR. |
Signed-off-by: Owais Kazi [email protected]
Description
Implemented Test Publisher to store result of the test to S3.
Test Publisher will be invoked by Test Recorder. Part of #340
Issues Resolved
Closes #341
Check List
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.