Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EXPECT_TIMELY test macro #3940

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

pwojcikdev
Copy link
Contributor

It is not always possible to call ASSERT_TIMELY, for example from helper functions or lambdas.

@pwojcikdev pwojcikdev requested review from dsiganos and thsfs September 1, 2022 21:31
@dsiganos
Copy link
Contributor

dsiganos commented Sep 1, 2022

Being the last checks makes them the same as ASSERT_TIMELY, doesn't it?
Looks good anyway.

dsiganos
dsiganos previously approved these changes Sep 1, 2022
thsfs
thsfs previously approved these changes Sep 1, 2022
@pwojcikdev pwojcikdev dismissed stale reviews from thsfs and dsiganos via dacdaab September 2, 2022 09:20
@pwojcikdev pwojcikdev force-pushed the prs/expect-timely-macro branch from 0229552 to dacdaab Compare September 2, 2022 09:20
{ \
ASSERT_NO_ERROR (system.poll ()); \
}
#define ASSERT_TIMELY(time, condition) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that it checks the condition twice. It should only check it once per loop.

For example, it won't catch this condition:

bool counter_increment_and_equals_ten()
{
    static int i = 0;
    return (--i) == 10;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was not the best solution. I decided to leave ASSERT TIMELY as is

@pwojcikdev pwojcikdev force-pushed the prs/expect-timely-macro branch from dacdaab to a1cddb9 Compare September 2, 2022 15:00
@pwojcikdev pwojcikdev requested a review from dsiganos September 2, 2022 15:05
/** Extends gtest with a std::error_code assert that expects an error */
#define ASSERT_IS_ERROR(condition) \
GTEST_TEST_ERROR_CODE ((condition.value () > 0), #condition, "An error was expected", "", \
GTEST_FATAL_FAILURE_)

/** Extends gtest with a std::error_code assert that expects an error */
#define EXPECT_IS_ERROR(condition) \
GTEST_TEST_ERROR_CODE ((condition.value () > 0), #condition, "An error was expected", "", \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

condition.value () != 0

protects against signed vs unsigned problems.

condition.value () > 0

#define EXPECT_NO_ERROR(condition) \
GTEST_TEST_ERROR_CODE (!(condition), #condition, condition.message ().c_str (), "", \
GTEST_NONFATAL_FAILURE_)

/** Extends gtest with a std::error_code assert that expects an error */
#define ASSERT_IS_ERROR(condition) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!= 0 is better than > 0

@pwojcikdev pwojcikdev force-pushed the prs/expect-timely-macro branch from 7b4bc1b to 15b5330 Compare September 5, 2022 12:31
@pwojcikdev pwojcikdev merged commit cf25128 into nanocurrency:develop Sep 5, 2022
@pwojcikdev pwojcikdev deleted the prs/expect-timely-macro branch September 5, 2022 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants