breaking(pytest_plugins/operator_groups): Raise exception if test missing group number #113
Conversation
…sing group number Fixes #106 If a test is missing a group number, it is almost certainly a mistake instead of intentional. Previously, if the test was missing a group number, it was marked as skipped (which will cause the job to pass on GitHub CI).
|
tested on canonical/mysql-operator#370 |
| @@ -60,7 +60,9 @@ class Group: | |||
| groups: set[Group] = set() | |||
| for function in items: | |||
| if (group_number := _get_group_number(function)) is None: | |||
| continue | |||
| raise Exception( | |||
There was a problem hiding this comment.
What if we add a fallback value + a deprecation warning instead?
So it allow a more gentle adoption.
There was a problem hiding this comment.
That defeats the whole point—the change in this PR is to fail loudly instead of quietly skipping the test
There was a problem hiding this comment.
FYI this change doesn't break anyone not using the pytest_operator_groups plugin
There was a problem hiding this comment.
My point was not to skip the test silently, but to fallback to something, let's say group 1 with the addition of a loud warning.
There was a problem hiding this comment.
Ah, I see
I was also thinking about adding that
My concern is that inferring the group number could get a bit too magical and make it difficult to debug
For example, if you add a new test at the end of a file with 2 test groups and forget a group number, it will be added to group 1. Without the plugin, the behavior with pytest-operator is to run the tests sequentially. Many of our tests are written in a way that assumes execution order (e.g. assumes that cluster is deployed). However, if you forget the group number, it will break developer's existing expectations about test ordering and run before any tests in group 2
IMO maybe still worth considering as a separate issue/PR
Regardless, I think this PR is an improvement on the current behavior (quietly skip tests)
Fixes #106
If a test is missing a group number, it is almost certainly a mistake instead of intentional. Previously, if the test was missing a group number, it was marked as skipped (which will cause the job to pass on GitHub CI).