Migrate from Catch to GoogleTest & always terminate on contract violation #831

JordanMaples · 2019-12-03T23:26:15Z

Eliminate throwing options from the contract violation behavior of Expects and Ensures. Contract violation will always result in termination.

Removes Catch as the unit testing framework to use GoogleTest instead.
Unit tests that test contract violation that previously relied on throw/catch logic will now be tested using EXPECT_DEATH.

JordanMaples · 2019-12-03T23:30:33Z

~~The build appears to be failing because of warnings emanating from within gtest itself.~~

~~Need to investigate why the warnings aren't being treated as "SYSTEM" as specified in the CMake file.~~

JordanMaples · 2019-12-03T23:49:59Z

~~MSVC x86 is failing, it appears that gtest is set to x64.~~

tests/CMakeLists.txt

JordanMaples · 2019-12-12T00:26:03Z

~~Update readme.md, it still lists catch as the test harness.~~

JordanMaples · 2019-12-12T00:27:15Z

~~Set terminate handlers for each of the death tests and emit a test relevant message to avoid the ".*" regex.~~

…assert. gsl_util to use terminate instead of throw_exception

sunnychatterjee

Hi Jordan,

Nice work in getting the changes together. I've left some comments for your consideration.

Thank you,
Sunny

sunnychatterjee · 2019-12-18T01:21:02Z

include/gsl/gsl_assert

-#define GSL_CONTRACT_CHECK(type, cond) GSL_ASSUME(cond)
-
-#endif // GSL_THROW_ON_CONTRACT_VIOLATION
-
 #define Expects(cond) GSL_CONTRACT_CHECK("Precondition", cond)


Expects(cond) and Ensures(cond): Should we remove the definitions using GSL_CONTRACT_CHECK?

Yes, the test failures will are easily accessible using the verbose output in the command line or using the test explorer in VS. With this information, there's no need to have the additional messages.
If a developer wants additional messages in their own execution, they can set up their own termination handler.

Yes, I'm comfortable removing them. The test failures will are easily accessible using the verbose output in the command line or using the test explorer in VS. With this information, there's no need to have the additional messages.
If a developer wants additional messages in their own execution, they can set up their own termination handler.

On second thoughts, let's just leave them as-is for now for the purposes of this PR.

sunnychatterjee · 2019-12-18T01:27:33Z

tests/CMakeLists.txt

    target_compile_options(gsl_tests_config_noexcept INTERFACE
        ${GSL_CPLUSPLUS_OPT}
+        /EHsc


Is this needed for the google test library?

yes, i believe it is needed by them. Before adding it, I was unable to get it to build.

tests/algorithm_tests.cpp

include/gsl/gsl_assert

tests/algorithm_tests.cpp

sunnychatterjee · 2019-12-18T01:55:56Z

tests/at_tests.cpp

@@ -133,3 +150,6 @@ static constexpr bool test_constexpr()
 static_assert(test_constexpr(), "FAIL");
 #endif

+#if __clang__ || __GNUC__


If the warnings are just in gtest header, this move up right after the include of the header.

sunnychatterjee · 2019-12-18T01:59:07Z

tests/at_tests.cpp

@@ -20,7 +20,18 @@
 #pragma warning(disable : 26440 26426) // from catch
 #endif

-#include <catch/catch.hpp> // for AssertionHandler, StringRef, CHECK_THROW...
+#if __clang__ || __GNUC__
+//disable warnings from gtest


Don't like warning suppressions in code. Are there issues tracking these warnings in gtest? If not, could you please file them?

Is there any way this snippet can be included in a common header since it's being repeated in a number of tests? I understand this might not be possible if the scope of suppression extends beyond the inclusion of gtest.h.

I can move them to a standalone header that also adds gtest.h. I'll add a comment with it's include so the corresponding GCC diagnostic pop at the bottom doesn't cause any confusion.

I decided against moving the push and suppression into a standalone header. This was due to the pop that is required at the end of the file. It struck me as odd that there would be a standalone pop. Even with comments, I wasn't satisfied with it.
Since there wasn't an elegant solution, I decided to leave them in the individual files.

JordanMaples · 2019-12-31T19:15:30Z

@sunnychatterjee Did I sufficiently address all of your comments and concerns?

As an additional note: I published an official release, v2.1.0, earlier today. Users who are against adopting the new contract violation behavior and test harness can opt to use that instead.

sunnychatterjee

Looks good to me. Thanks!

reddwarf69 · 2020-01-28T09:36:23Z

What was the logic behind this?

It seems to imply a history like:

Everybody accepts Catch is better than Google Test, so you went with Catch
But Catch doesn't support death tests, so GSL_THROW_ON_CONTRACT_VIOLATION was added
And since there was an option why not also add GSL_UNENFORCED_ON_CONTRACT_VIOLATION?
.... time passes ...
Having the options is a problem because... some reason?
You want to remove the options so badly that you are willing to use Google Test, even if you prefer Catch otherwise.

But it's unclear what "some reason" is.

From outside what it looks like is:

The options are being removed for no reason
It breaks anyone's code that relied on GSL_THROW_ON_CONTRACT_VIOLATION to use Catch to detect contract violations using GSL.
It removes the option to disable the checks if somebody wants to have them only enabled in tests but not in production.

grahamreeds · 2020-01-28T09:51:04Z

And prior to this they were using UnitTest++. Was there something missing from UnitTest++? Programmers always want to use the new hotness, even if the gains are marginal. GR

…

On Tue, 28 Jan 2020, 09:36 Cristian Morales Vega, ***@***.***> wrote: What was the logic behind this? It seems to imply a history like: - Everybody accepts Catch is better than Google Test, so you went with Catch - But Catch doesn't support death tests, so GSL_THROW_ON_CONTRACT_VIOLATION was added - And since there was an option why not also add GSL_UNENFORCED_ON_CONTRACT_VIOLATION? - .... time passes ... - Having the options is a problem because... some reason? - You want to remove the options so badly that you are willing to use Google Test, even if you prefer Catch otherwise. But it's unclear what "some reason" is. From outside what I looks like is: - The options are being removed for no reason - It breaks anyone's code that relied on GSL_THROW_ON_CONTRACT_VIOLATION to use Catch to detect contract violations using GSL. - It removes the option to disable the checks if somebody wants to have them only enabled in tests but not in production. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#831?email_source=notifications&email_token=ABGL3HDV53J3ERR3CI5IZKLQ774CFA5CNFSM4JVAF7D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKCUDXI#issuecomment-579158493>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGL3HGE6DDSGPFPE4SW66TQ774CFANCNFSM4JVAF7DQ> .

JordanMaples · 2020-01-28T18:53:39Z

The goal of this PR was to align the GSL's contract violation behaviors to those outlined in the Core Guidelines. Specifically, in isocpp/CppCoreGuidelines#1512 it was discussed that contract violation should result in terminating behavior. So removal of the alternative contract violation behavior was necessary. I created a snapshot of the GSL just before this PR was merged in case you would like to have the old behavior. https://github.com/microsoft/GSL/releases/tag/v2.1.0

@grahamreeds Unfortunately, I was not involved with the GSL during the migration from UnitTest++ to Catch and am unfamiliar with UnitTest++.

irrenhaus3 · 2022-01-04T21:42:30Z

New year, new comment: I found that the (new?) wording on https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-assertions states that Expects is allowed an alternative to program termination. I'd like to request the return of fail_fast or a similar exception so that unit test frameworks licke Catch that aren't capable of Death Tests can still test for contract violations that way.

beinhaerter · 2022-01-05T22:19:57Z

I found that the (new?) wording on https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-assertions states that Expects is allowed an alternative to program termination.

This is no new wording, this is the same wording since 2015 (Compare git log --topo-order --graph -u -L 21233,21245:CppCoreGuidelines.md).

The latest discussion linked by JordanMaples is from September 2019. This is 4 years after the last change to the GSL.assert section in the CoreGuidelines.

Regarding your change request I agree that the status is unfortunate. As reddwarf69 points out in isocpp/CppCoreGuidelines#1512 (comment) there are open questions to which hsutter has not yet replied.

CoreGuidelines text says "Expects(p) terminates the program unless p == true" which is the default behaviour and it says "Expects is under control of some options (enforcement, error message, alternatives to terminate)" which would allow to configure other behaviour
The CoreGuidelines editors say "GSL.assert currently requires Expects to terminate the program if the condition is false. This is intended to mean std::terminate which permits a terminate_handler to do things like log some final information before restarting the process to try again. There is nothing in the Guidelines today about Expects throwing on contract violations." where the last sentence in my opinion contradicts the text of the CoreGuidelines
The CoreGuideline issue is still open, so hsutter's answer might not have been the final answer
The GSL code was changed directly in reply to the editors call because "removal of the alternative contract violation behavior was necessary." which seems a bit eager especially since the CoreGuideline issue is not yet closed

I asked hsutter in the CoreGuidelines issue if he can reply to the open questions. Maybe this helps close the discussion.

JordanMaples added 2 commits December 3, 2019 14:32

gtest migration

2b10729

removing unused function

603b467

JordanMaples and others added 6 commits December 4, 2019 12:30

ignore warnings from gtest

43aca26

suppressing -Wundef in clang (GTest emitted)

87eaa45

attempting to fix the gtest print issues in vs17

b39a973

fixing suppression of gtest warnings for gcc

9a88262

added back death test, fixed VS2019 test failures

0784866

dummy commit to trigger appveyor

ec05ecd

JordanMaples changed the title ~~[DRAFT] Migrate from Catch to GoogleTest~~ Migrate from Catch to GoogleTest Dec 10, 2019

JordanMaples changed the title ~~Migrate from Catch to GoogleTest~~ Migrate from Catch to GoogleTest & always terminate on contract violation Dec 11, 2019

JordanMaples commented Dec 11, 2019

View reviewed changes

tests/CMakeLists.txt Outdated Show resolved Hide resolved

JordanMaples added 6 commits December 12, 2019 10:55

changing text in notices & readme. adding terminate handler for tests

4f6f05d

removed string_view for string in tests

5099e8f

appveyor failed to start, sending dummy commit

0931262

change from string to char*

6c5c708

deprecate gsl::details::throw_exception. remove unused header in gsl_…

17ad449

…assert. gsl_util to use terminate instead of throw_exception

adding /EHsc for msvc cmake target gsl_test_config_noexcept

3539bd8

sunnychatterjee reviewed Dec 18, 2019

View reviewed changes

JordanMaples added 2 commits December 19, 2019 14:05

addressing comments

8577033

testing something

40bc3c3

sunnychatterjee approved these changes Jan 7, 2020

View reviewed changes

JordanMaples merged commit 7786da9 into microsoft:master Jan 7, 2020

JordanMaples deleted the dev/jomaples/gtest branch January 10, 2020 19:14

reddwarf69 mentioned this pull request Jan 28, 2020

Should Expects be required to abort? isocpp/CppCoreGuidelines#1512

Open

JordanMaples mentioned this pull request Jan 30, 2020

Clarification requested for Expects/Ensures contract violation behavior isocpp/CppCoreGuidelines#1561

Open

unixod mentioned this pull request Apr 24, 2020

gsl::not_null can't be anymore zero cost abstraction. #877

Closed

robert-andrzejuk mentioned this pull request Apr 29, 2020

Implement process isolation support for tests catchorg/Catch2#853

Open

JohelEGP mentioned this pull request Aug 13, 2020

Error Handling with Core Guidelines GSL Expects, Ensures, and narrow_cast isocpp/CppCoreGuidelines#1663

Open

JordanMaples mentioned this pull request Jul 8, 2021

gsl::fail_fast not found in the namespace #988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from Catch to GoogleTest & always terminate on contract violation #831

Migrate from Catch to GoogleTest & always terminate on contract violation #831

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 12, 2019 •

edited

Loading

JordanMaples commented Dec 12, 2019 •

edited

Loading

sunnychatterjee left a comment

sunnychatterjee Dec 18, 2019

JordanMaples Dec 19, 2019

JordanMaples Dec 19, 2019

sunnychatterjee Jan 7, 2020

sunnychatterjee Dec 18, 2019

JordanMaples Dec 19, 2019

sunnychatterjee Dec 18, 2019

sunnychatterjee Dec 18, 2019

JordanMaples Dec 18, 2019

JordanMaples Dec 19, 2019 •

edited

Loading

JordanMaples commented Dec 31, 2019

sunnychatterjee left a comment

reddwarf69 commented Jan 28, 2020 •

edited

Loading

grahamreeds commented Jan 28, 2020 via email

JordanMaples commented Jan 28, 2020

irrenhaus3 commented Jan 4, 2022

beinhaerter commented Jan 5, 2022

Migrate from Catch to GoogleTest & always terminate on contract violation #831

Migrate from Catch to GoogleTest & always terminate on contract violation #831

Conversation

JordanMaples commented Dec 3, 2019 • edited Loading

JordanMaples commented Dec 3, 2019 • edited Loading

JordanMaples commented Dec 3, 2019 • edited Loading

JordanMaples commented Dec 12, 2019 • edited Loading

JordanMaples commented Dec 12, 2019 • edited Loading

sunnychatterjee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanMaples Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

JordanMaples commented Dec 31, 2019

sunnychatterjee left a comment

Choose a reason for hiding this comment

reddwarf69 commented Jan 28, 2020 • edited Loading

grahamreeds commented Jan 28, 2020 via email

JordanMaples commented Jan 28, 2020

irrenhaus3 commented Jan 4, 2022

beinhaerter commented Jan 5, 2022

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 3, 2019 •

edited

Loading

JordanMaples commented Dec 12, 2019 •

edited

Loading

JordanMaples commented Dec 12, 2019 •

edited

Loading

JordanMaples Dec 19, 2019 •

edited

Loading

reddwarf69 commented Jan 28, 2020 •

edited

Loading