[SERVER-53380] Improve assert_util classes and implementation Created: 15/Dec/20 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Tyler Seip (Inactive) | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 1 |
| Labels: | sa-remove-fv-backlog-22 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Assigned Teams: |
Service Arch
|
||||||||||||
| Participants: | |||||||||||||
| Description |
|
In
|
| Comments |
| Comment by Billy Donahue [ 09/Mar/21 ] | |||||||||||
|
So far everything on this ticket has been about tassert and AssertionCount. It would be good to unify the assert macros through a central MONGO_BASE_ASSERT_VA_DISPATCH.
There are numerous small things like:
Some of these are addressed by this PoC: | |||||||||||
| Comment by Billy Donahue [ 09/Mar/21 ] | |||||||||||
|
A few thoughts here.
| |||||||||||
| Comment by Benjamin Caimano (Inactive) [ 21/Dec/20 ] | |||||||||||
|
I think perhaps the biggest difference would be that you would still potentially mark the suite and test that encountered the tripwire as without failure. Devs tend to be rightfully frustrated when they experience "failure" after being told there was "success". We also need to reset the counts for the tripwire tests themselves, but we can make a specific fixture for that. | |||||||||||
| Comment by Billy Donahue [ 16/Dec/20 ] | |||||||||||
It would not be difficult unless I'm missing something here. Here's a piece of util_test on some evergreen run:
The tripwire failure emits a log line with a conspicuous id number that would appear in the unit test output. | |||||||||||
| Comment by Benjamin Caimano (Inactive) [ 15/Dec/20 ] | |||||||||||
|
I believe the general concern was that it would be difficult to associate tripwire failures with individual suites given how big some of our linked unittests are. For better or worse, it did slip past my initial review. I noticed it again when I was evaluating the potential for a backport. | |||||||||||
| Comment by Billy Donahue [ 15/Dec/20 ] | |||||||||||
|
Yeah it's really important that the unittest::Test base class setUp and tearDown should not be doing any work at all. That should not have passed review, really. Is it really necessary to check for tripwireAssertions after each TEST/TEST_F? Really it could just be an atexit lazy-added from the tassert. | |||||||||||
| Comment by Benjamin Caimano (Inactive) [ 15/Dec/20 ] | |||||||||||
|
If we're doing a laundry list, we should probably move the tripwire assertion updates in our unittests here to the unittest::Test ctor/dtor instead. People tend to override those functions which would thus disable the tripwire checks. |