test(backup): Remove @targets decorator and improve assert message #61719
Conversation
Two improvements to the backup test scaffolding are included in this PR: 1. The `@targets` decorator is replaced with `@expect_models`, which is simpler and does not rely on test function returns, as these are deprecated in Python 3.11 2. The error message shown to developers (and in CI) when the new `verify_models_in_output` test function fails are much more detailed, providing an explanation of what went wrong and where to look to fix the problem.
tests/sentry/backup/__init__.py
Outdated
| the output, ensuring that we're actually testing the thing we think we are. Additionally, this | ||
| decorator is easily legible to static analysis, which allows for static checks to ensure that | ||
| all `__relocation_scope__ != RelocationScope.Excluded` models are being tested. | ||
| def verify_models_in_output(expected_models: list[Type[models.Model]], actual_json: JSONData): |
There was a problem hiding this comment.
this should be -> None (otherwise mypy doesn't opt this into type checking)
| def decorator(func: Callable): | ||
| @wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| return func(*args, target_models, **kwargs) |
There was a problem hiding this comment.
I'd also discourage signature-modifying decorators -- they're still action-at-a-distance and it'd be much more explicit to use a context manager
There was a problem hiding this comment.
The reason I want to use a decorator here is because test_coverage.py checks these for completeness statically. If we use a context manager, the ..._TESTED globals only get populated at test runtime.
There was a problem hiding this comment.
is there another way you can satisfy that without metaprogramming?
There was a problem hiding this comment.
I suppose there always is, but it's a bigger lift than this change as constituted.
The main case we're trying to guard against is developers not thinking about backups when they add/modify models, thereby causing new models to be untested. If we tested all of the cases in a single large test then meta-programming would be unnecessary and we could just check at test runtime, but that would make for very large and unwieldy test cases (ex: we have ~10 collision tests that use COLLISION_TESTED, which make much more sense when split by model than if they were glued together into a single giga-test).
There was a problem hiding this comment.
Hi, just following up here. While the change as constituted may not be perfect, it is better than the current state of affairs (simpler decorator functionality), and fixes two relatively more pressing issues:
- Unblocking future python version bumps by not returning from tests.
- Providing devs with a clearer error message when they trip this check in CI.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #61719 +/- ##
========================================
Coverage 81.14% 81.14%
========================================
Files 5191 5188 -3
Lines 228313 228526 +213
Branches 38275 38330 +55
========================================
+ Hits 185261 185441 +180
- Misses 37432 37456 +24
- Partials 5620 5629 +9 |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
this regressed this: |
asottile-sentry
added
the
Trigger: Revert
|
PR reverted: 2f81e77 |
…ssage (#61719)" This reverts commit bbd0230. Co-authored-by: asottile-sentry <[email protected]>
Two improvements to the backup test scaffolding are included in this PR:
@targetsdecorator is replaced with@expect_models, which is simpler and does not rely on test function returns, as these are deprecated in Python 3.11.verify_models_in_outputtest function fails are much more detailed, providing an explanation of what went wrong and where to look to fix the problem.