[SERVER-6317] Make assertions behave uniformly in debug and normal builds Created: 05/Jul/12 Updated: 28/Aug/23 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Testing Infrastructure |
| Affects Version/s: | 2.1.2 |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Randolph Tan | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Assigned Teams: |
Service Arch
|
||||||||||||||||
| Participants: | |||||||||||||||||
| Description |
|
Issue is that, since we abort on some of the assertion failure (only in DEBUG build), tests that tries to check whether an assertion is activated will fail because it aborts the mongo process instead of just throwing the AssertionException. |
| Comments |
| Comment by Steven Vannelli [ 10/May/22 ] |
|
Moving this ticket to the Backlog and removing the "Backlog" fixVersion as per our latest policy for using fixVersions. |
| Comment by Andrew Morrow (Inactive) [ 20/Jun/18 ] |
|
Thanks max.hirschhorn - sent it over to your team, still in needs triage. |
| Comment by Max Hirschhorn [ 20/Jun/18 ] |
|
I think the TIG team can take it and send the Platforms team a code review. |
| Comment by Andrew Morrow (Inactive) [ 20/Jun/18 ] |
|
max.hirschhorn - I'm fine with that approach. Are you hoping that platforms would implement that, or is that within TIGs scope of activity, in which case I'm sure platforms is willing to help with review. |
| Comment by Max Hirschhorn [ 20/Jun/18 ] |
|
I think we should add -DVERIFY_IS_INVARIANT and try to stand up a new build variant in Evergreen. We can probably stage it incrementally with what Evergreen tasks we run on it. For instance, we currently don't run the jstestfuzz* Evergreen tasks against DEBUG builds because we'd otherwise hit verify() failures. |
| Comment by Andrew Morrow (Inactive) [ 19/Jun/18 ] |
|
max.hirschhorn - I've moved this to 'Needs Triage' so we can discuss in our team triage meeting, but I'm sort of the opinion that we should kick this back to TIG to decide which way they want this resolved - I've shared my thoughts on the options. We can also decide to just keep on doing what we have been doing for 6 years and just close this as Won't Fix. |
| Comment by Andrew Morrow (Inactive) [ 14/Jun/18 ] |
|
I don't like verify because when you see one in the code you can't know what was originally meant. Did the original author really mean invariant, meaning that the impossible happened, but reached for the wrong tool and wrote verify? Or did they mean dassert but were too scared to continue in release mode, and therefore should have written something stronger? Or did they mean uassert, in that the operation should fail, which is what happens in release mode, but they wanted tests to die? The whole thing is a mess. I think we need to commit to bulk converting all verify calls to either invariant or uassert. The former choice is the aggressive one (user reachable verify calls will now become crashes), and would destabilize things until we found the places where verify was really supposed to mean uassert. Moving verify to uassert is the conservative choice, in that there will be no new unintended crashing paths. I think the decision should lie with the TIG team as to which way to play it. Either way, we could definitely take your suggestion of standing up a build that enforces that choice, via a -DVERIFY_IS_INVARIANT or -DVERIFY_IS_UASSERT. Or do both and get them both green? |
| Comment by Max Hirschhorn [ 26/May/18 ] |
|
acm, what would be your recommendation to convert existing verify() calls to the appropriate invariant() or uassert() calls? |