-
Notifications
You must be signed in to change notification settings - Fork 273
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 smoke tests to the test manifest schema #5127
Conversation
Signed-off-by: Zelin Hao <[email protected]>
src/manifests/test_manifest.py
Outdated
@@ -36,6 +36,9 @@ class TestManifest(ComponentManifest['TestManifest', 'TestComponents']): | |||
test-configs: | |||
- with-security | |||
- without-security | |||
smoke-test: | |||
test-spec: |
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.
Do we want to keep this as test-configs
? Similar to integ-test and bwc test?
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.
I'm thinking it's not technically config? Keep it as test-configs
may cause some confusion to users since we don't modify any of the cluster configuration.
src/manifests/test_manifest.py
Outdated
"smoke-test": { | ||
"type": "dict", | ||
"schema": { | ||
"test-spec": {"type": "list"}, |
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.
Do we need to use list here? More than 1 spec file for each test?
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.
We can start with one spec per component and then iterate if we need more. Would recommend to add multiple specs under same file rather than having multiple spec files for one component.
Sure let me update it to a single string of spec for now. We can enhance later if we really need more. Thanks. |
Signed-off-by: Zelin Hao <[email protected]>
src/manifests/test_manifest.py
Outdated
@@ -37,8 +37,7 @@ class TestManifest(ComponentManifest['TestManifest', 'TestComponents']): | |||
- with-security | |||
- without-security | |||
smoke-test: | |||
test-spec: | |||
- spec.yaml | |||
test-spec: spec.yaml |
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.
Can we keep it consistent here?
.yaml
or .yml
?
Thanks.
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.
Sure updated to .yml
.
Signed-off-by: Zelin Hao <[email protected]>
@gaiksaya @peterzhuamazon Any final thoughts or concerns before we merge this PR? |
LGTM, thanks. |
Description
Add smoke tests to the test manifest schema
Issues Resolved
#5126
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.