-
Notifications
You must be signed in to change notification settings - Fork 87
Conversation
🤖 zincr found 0 problems , 0 warnings
|
🤖 zincr found 1 problem , 0 warnings
Details on how to resolve are provided below ApprovalsAll proposed changes must be reviewed by project maintainers before they can be merged Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.
|
1 similar comment
🤖 zincr found 1 problem , 0 warnings
Details on how to resolve are provided below ApprovalsAll proposed changes must be reviewed by project maintainers before they can be merged Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.
|
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.
Just a nitpick outside of this PR's diff!
e2e_test_creation = bool(m) | ||
m = re.search(r'@kopf.on.(create|update|delete)\(', example_py.read_text(), re.M) | ||
e2e_test_highlevel = bool(m) | ||
|
||
# check whether there are mandatory deletion handlers or not | ||
m = re.search(r'@kopf\.on\.delete\((\s|.*)?(optional=(\w+))?\)', example_py.read_text(), re.M) |
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.
This (\s|.*)?
seems unnecessarily complicated, as it is equivalent to .*
with the re.S
flag, creates an unused capturing group, and doesn't match multiple newlines, comments, etc.
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 do not like this approach with the source code parsing with regular expressions at all. Especially when new criteria are added, as in this case. This "examples as e2e tests" approach should be revised to something else, but I don't know to what (yet). Any ideas are welcome. The goal is to test that the examples are still relevant and functional, while keeping in mind that they all are different.
Fix the never-ending handling cycle, and fix the tests to catch this.
Description
These changes below could not be separated into their own PRs, as fixing the tests breaks the operator's CI/CD pipeline (the code is broken in master now), and fixing the issue is not provable (the tests are false-positive in master now). So, they go as one PR with two topics.
Part 1:
While adding the type-annotations in #200, some data structures were split and their handling was reimplemented slightly. In particular, full body and body essence were separated -- to be used in different places and to make it clear that they have different content.
By intention, the essence is a deep copy of the body (there is a line to do that).
However, in the body's essence calculation, labels and annotations were taken from the original body as is, as the
dict
objects, and the annotations were modified in place to remove the system's/framework's ones -- which broke the intention of the essence being a deep copy of the body.As a result, the important annotations of the last-seen state were lost also from the body (not only from the essence), and the framework could not detect the cause properly in case of the multi-handler operators (with at least 2 handling cycles), so it always assumed that this is a newly created object (due to absence of the last-seen state), and repeated the handling non-stop, again and again.
The infinite handling during 2 seconds of the e2e tests was never checked and asserted, so it went unnoticed, despite it was logged and captured.
This case is now prevented from happening in the future (beside the fix) by checking how many times every handler-of-interest has succeeded/failed.
Part 2:
While fixing the above-mentioned issue, it was found that the e2e tests were working just by luck: they took the global registry of the previously executed tests, added new handlers, and for this reason could find all the needed log lines in the output -- they were coming from the previous tests, not from the current one.
This issue is now fixed by moving the
clear_default_registry
fixture to the top-level conftest plugin, and is now auto-applied for all the tests (both unit and e2e).Once this "test overlapping" effect was removed, few e2e tests actually broke for valid reasons. So, they had to be fixed with proper timing.
Types of Changes