[SERVER-25698] ASSERT_NOT alias for ASSERT_FALSE Created: 19/Aug/16  Updated: 06/Dec/22

Status: Backlog
Project: Core Server
Component/s: Testing Infrastructure
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Kevin Pulo Assignee: Backlog - Server Tooling and Methods (STM) (Inactive)
Resolution: Unresolved Votes: 0
Labels: stm, tig-unittests
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
Assigned Teams:
Server Tooling & Methods
Backwards Compatibility: Fully Compatible
Participants:

 Description   

In unittest/unittest.h, ASSERT_TRUE has ASSERT as an alias for readability.

In a similar vein, some tests would benefit (in terms of readability) from having an ASSERT_NOT macro, which is an alias for ASSERT_FALSE.

For example, I think this:

ASSERT_NOT(::mongo::serverGlobalParams.cwd.empty());

is more readable (ie. more clearly expresses the concept that "cwd is not empty") than either of the alternatives:

// Slightly brain bending
ASSERT_FALSE(::mongo::serverGlobalParams.cwd.empty());

// The ! is easily missed (visually)
ASSERT(!::mongo::serverGlobalParams.cwd.empty());



 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 Kevin Pulo [ 03/Feb/21 ]

I completely agree that that the assert_that framework is a much better direction. Feel free to repurpose this ticket for that, if you'd like.

Comment by Billy Donahue [ 21/Dec/20 ]

I would rather have a minimal set of well-named macros than a proliferation of aliases like this.
I would like to go the other direction and get rid of ASSERT_FALSE.
We always have to read the expression carefully.
The difficulty in that sample expression: ASSERT(!::mongo::serverGlobalParams.cwd.empty()); is the amount of punctuation and leading global scope operator ::. That operator is not typically necessary, only in macros typically.

 
ASSERT(!::mongo::serverGlobalParams.cwd.empty()); // too many hiding places, but also very explicit
 
ASSERT(!mongo::serverGlobalParams.cwd.empty()); // better!
 
ASSERT(!serverGlobalParams.cwd.empty()); // sufficient, and most code is already in mongo::.

All ASSERT macros have the ability to attach extra information and this isn't used enough!
(actually only the BSON comparison asserts don't but they will hopefully be repaired soon fixed by SERVER-52682 )

ASSERT(!serverGlobalParams.cwd.empty()) << "should not be empty";

I think the better way forward is to have composable matchers for ASSERT and then very few variants of ASSERT.
WRITING-7114 will try to scope this. PoC https://github.com/BillyDonahue/mongo/tree/assert_that

Generated at Thu Feb 08 04:09:56 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.