Repository navigation
[uss_qualifier/reports] Finish removing successful property - #1822
Conversation
BenjaminPelletier
added
P3
| failed_checks = [check for _, check in report.query_failed_checks()] | ||
| assert len(failed_checks) == 1 | ||
| assert failed_checks[0].name == "test-check-c-5-critical" | ||
| assert failed_checks[0].severity == Severity.Critical |
There was a problem hiding this comment.
Nit: I'm not sure the whole set of tests are needed anymore, since they are just testing now for query_failed_checks. It can probably be factorized into single tests / removed.
There was a problem hiding this comment.
It seems like the tests still have valid coverage (not made tautological by the changes). It seems like a number of tests could be merged/refactored, but I don't think we should gate progress on optimally reconfiguring tests. If the expectation were that any changes that affect a test also require or recommend optimizing the test in the same PR, I think the bar for adding tests would be much higher since they would impose development drag when written suboptimally. I don't think that would be optimal, so it seems like the expected burden regarding existing tests during development should simply be to make sure they pass while still making sense, or else remove them if they're no longer applicable. I think those have all been satisfied here.
This PR resolves #1428.
This PR pays off tech debt and isn't urgent.