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
Fail tests without non-empty @cluster annotation #336
Fail tests without non-empty @cluster annotation #336
Conversation
Important to note here that this WILL break people who do not use the cluster annotations tests. I would argue that this should be a flag and the default behavior should still be the same (as cluster annotation should really only be enforced when running with parallelism which isn't the default, w/o parallelism theres no harm no foul really) |
It'll take some time to sort these issues out so I think having a flag makes sense. |
I agree with not wanting to break everything, but I don't want to introduce a flag, purely because implementing and unit testing the flag will be a relative chore. |
@imcdo It's a different question about not enforcing this without parallelism though - I'll look into whether the behavior can be adjustable based on that. |
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.
Looks good thanks stan!
@lbradstreet @imcdo even though this seems to be ready, I'm not going to merge it just yet - a quick search across kafka and confluent tests reveals that there are some tests that still use min_cluster_spec() method (which is removed in this version). I'll either update those tests, or lock ducktape minor version requirement for those repos before merging this. |
Actually, I think a better thing to do would be to keep min_cluster_size method for compatibility, just hardcode it to return 0 and mark it as deprecated - this way we won't have to push a ton of PRs to multiple AK branches, they'll just continue to work. |
1. Deprecation of min_cluster_spec() method Turns out we have two different mechanisms to state expected number of nodes for the test - there's the @cluster annotation, which we know and love/hate, and min_cluster_spec method on the test object itself. They are not related, and we mostly use @cluster annotation, except for a single place where we check min_cluster_spec and possibly fail - however, this check makes no sense, because it has already been checked against @cluster annotation, plus the test would fail if it tries to allocated too many nodes anyway. So I'm deprecating min_cluster_spec method, removing all its two usages (the second one is for reporting, and it's also inaccurate). The method is still there, so if clients override it and refer to super() they should still be ok. They would probably be ok even if I remove it, since python is interpreted and if nothing calls that method, it's probably fine, but it's cleaner and more obvious this way. 2. Option to fail tests that don't explicitly specify their cluster requirements The real functionality of this PR is disallowing the tests that don't have @cluster annotation (or have them empty) and failing them immediately - since @cluster annotation is our preferred way of doing things right now, and since this protects us against tests that automatically use the whole cluster. This behavior is controlled by a new flag - --fail-greedy-tests, which enables this behavior when provided. We've discussed making this behavior automatic when using --parallel but ultimately felt that such behavior will be surprising for users, and not in a good way. We're still open to do this in the future, just not in this PR. This PR also explicitly allows (and tests for) empty cluster spec in @cluster() annotation (e.g. @cluster(num_nodes=0)), thus allowing tests that don't use any cluster resources. This functionality was working fine before this PR, but didn't have unit tests, so we've made sure it still works after this PR.
1. Deprecation of min_cluster_spec() method Turns out we have two different mechanisms to state expected number of nodes for the test - there's the @cluster annotation, which we know and love/hate, and min_cluster_spec method on the test object itself. They are not related, and we mostly use @cluster annotation, except for a single place where we check min_cluster_spec and possibly fail - however, this check makes no sense, because it has already been checked against @cluster annotation, plus the test would fail if it tries to allocated too many nodes anyway. So I'm deprecating min_cluster_spec method, removing all its two usages (the second one is for reporting, and it's also inaccurate). The method is still there, so if clients override it and refer to super() they should still be ok. They would probably be ok even if I remove it, since python is interpreted and if nothing calls that method, it's probably fine, but it's cleaner and more obvious this way. 2. Option to fail tests that don't explicitly specify their cluster requirements The real functionality of this PR is disallowing the tests that don't have @cluster annotation (or have them empty) and failing them immediately - since @cluster annotation is our preferred way of doing things right now, and since this protects us against tests that automatically use the whole cluster. This behavior is controlled by a new flag - --fail-greedy-tests, which enables this behavior when provided. We've discussed making this behavior automatic when using --parallel but ultimately felt that such behavior will be surprising for users, and not in a good way. We're still open to do this in the future, just not in this PR. This PR also explicitly allows (and tests for) empty cluster spec in @cluster() annotation (e.g. @cluster(num_nodes=0)), thus allowing tests that don't use any cluster resources. This functionality was working fine before this PR, but didn't have unit tests, so we've made sure it still works after this PR.
This might be breaking change if you rely on min_cluster_spec() method and also two changes in one.
1. Deprecation of
min_cluster_spec()
methodTurns out we have two different mechanisms to state expected number of nodes for the test - there's the
@cluster
annotation, which we know and love/hate, and min_cluster_spec method on the test object itself. They are not related, and we mostly use@cluster
annotation, except for a single place where we checkmin_cluster_spec
and possibly fail - however, this check makes no sense, because it has already been checked against@cluster
annotation, plus the test would fail if it tries to allocated too many nodes anyway.So I'm deprecating
min_cluster_spec
method, removing all its two usages (the second one is for reporting, and it's also inaccurate). The method is still there, so if clients override it and refer to super() they should still be ok. They would probably be ok even if I remove it, since python is interpreted and if nothing calls that method, it's probably fine, but it's cleaner and more obvious this way.2. Option to fail tests that don't explicitly specify their cluster requirements
The real functionality of this PR is disallowing the tests that don't have
@cluster
annotation (or have them empty) and failing them immediately - since@cluster
annotation is our preferred way of doing things right now, and since this protects us against tests that automatically use the whole cluster.This behavior is controlled by a new flag - --fail-greedy-tests, which enables this behavior when provided. We've discussed making this behavior automatic when using
--parallel
but ultimately felt that such behavior will be surprising for users, and not in a good way. We're still open to do this in the future, just not in this PR.This PR also explicitly allows (and tests for) empty cluster spec in
@cluster()
annotation (e.g.@cluster(num_nodes=0)
), thus allowing tests that don't use any cluster resources. This functionality was working fine before this PR, but didn't have unit tests, so we've made sure it still works after this PR.