-
Notifications
You must be signed in to change notification settings - Fork 592
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
tests: ability to dynamically parametrize cloud_storage_type #9283
Conversation
1893c52
to
ef4042a
Compare
3d83eb1
to
407f3d5
Compare
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.
LGTM
pre-emptive lgtm because tests are green and this PR now points to the changes that will be brought in with redpanda-data/ducktape#24
nit: update the git commit to the merge commit after PR 24 merges, but not a blocker for this PR
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.
Cool stuff
407f3d5
to
8241caa
Compare
Replaced `parametrized` with `matrix` decorator, and the cloud_storage_type holds the parametrization needed for each host type. For example for cdt runs on aws, the matrix will only contain s3 as cloud storage type. same for gcp or azure. For regular tests (host=docker container), it parametrizes with both s3 and abs ref redpanda-date/redpanda#8704
To be able to mark tests with empty list of `matrix` parametrization as `ignore` ref redpanda#8704 redpanda-data/ducktape#24
8241caa
to
773b44f
Compare
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.
Very nice. Should we trigger a cdt run to check if it works as expected, or have you tried it yourself?
@@ -169,6 +167,46 @@ def one_or_many(value): | |||
return value | |||
|
|||
|
|||
def get_cloud_storage_type(applies_only_on: list(CloudStorageType) = None, |
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: the type hint doesn't match. Should be Optional[..]
The latest report I ran is https://ci-artifacts.dev.vectorized.cloud/vtools/6695/0186dfe2-dbea-4e04-869c-25850ec94a48/vbuild/ducktape/results/2023-03-14--001/report.html where there is no |
Replaced
parametrized
withmatrix
decorator,and the cloud_storage_type holds the parametrization
needed for each host type. For example for
cdt runs on aws, the matrix will only contain s3
as cloud storage type. same for gcp or azure.
For regular tests (host=docker container), it
parametrizes with both s3 and abs
Additionally, tests that are parametrized to run on a single
cloud storage type, will be explicitly excluded from the test
collection ref: https://github.com/redpanda-data/vtools/pull/1554/commits/131144e09460a07afc690dd4a1ab65a507b41f2d
ref #8704
Backports Required
Release Notes