pgsql: Report test_atomic_ops() failures consistently, via macros.

Hi,

On 2019-10-05 17:08:38 +0000, Noah Misch wrote:

Report test_atomic_ops() failures consistently, via macros.

This prints the unexpected value in more failure cases, and it removes
forty-eight hand-maintained error messages. Back-patch to 9.5, which
introduced these tests.

Thanks for these, that's a nice improvement.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=639feb92e1186e1de495d1bfdb191869cb56450a

...
+#define EXPECT_EQ_U32(result_expr, expected_expr)  \
+   do { \
+       uint32      result = (result_expr); \
+       uint32      expected = (expected_expr); \
+       if (result != expected) \
+           elog(ERROR, \
+                "%s yielded %u, expected %s in file \"%s\" line %u", \
+                #result_expr, result, #expected_expr, __FILE__, __LINE__); \
+   } while (0)
...

I wonder if we should put these (and a few more, for other types), into
a more general place. I would like to have them for writing both tests
like regress.c:test_atomic_ops(), and for writing assertions that
actually display useful error messages. For the former it makes sense
to ERROR out, for the latter they ought to abort, as currently.

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

Greetings,

Andres Freund

On Sat, Oct 05, 2019 at 12:07:29PM -0700, Andres Freund wrote:

+#define EXPECT_EQ_U32(result_expr, expected_expr)  \
+   do { \
+       uint32      result = (result_expr); \
+       uint32      expected = (expected_expr); \
+       if (result != expected) \
+           elog(ERROR, \
+                "%s yielded %u, expected %s in file \"%s\" line %u", \
+                #result_expr, result, #expected_expr, __FILE__, __LINE__); \
+   } while (0)
...

I wonder if we should put these (and a few more, for other types), into
a more general place. I would like to have them for writing both tests
like regress.c:test_atomic_ops(), and for writing assertions that
actually display useful error messages. For the former it makes sense
to ERROR out, for the latter they ought to abort, as currently.

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

Sounds reasonable. For broader use, I would include the expected value, not
just expected_expr:

elog(ERROR, \
"%s yielded %u, expected %s (%u) in file \"%s\" line %u", \
#result_expr, result, #expected_expr, expected, __FILE__, __LINE__); \

I didn't do that for the atomics tests, where expected_expr is always trivial.
The codebase has plenty of Assert(x == y) where either of x or y could have
the surprising value.

Andres Freund <andres@anarazel.de> writes:

On 2019-10-05 17:08:38 +0000, Noah Misch wrote:

Report test_atomic_ops() failures consistently, via macros.

I wonder if we should put these (and a few more, for other types), into
a more general place. I would like to have them for writing both tests
like regress.c:test_atomic_ops(), and for writing assertions that
actually display useful error messages. For the former it makes sense
to ERROR out, for the latter they ought to abort, as currently.

IMO, anything named like "assert" ought to act like Assert does now,
ie (1) it's a no-op in a non-assert build and (2) you get an abort()
on failure. No strong opinions about what the test-and-elog variant
should be called -- but it seems like we might have some difficulty
agreeing on what the appropriate error level is for that. If it's
morally like an Assert except we want it on all the time, should
it be PANIC? What will happen in frontend code?

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

I'd just put them all in c.h. I see no reason why a new header
is helpful.

regards, tom lane

On 2019-10-06 04:20, Noah Misch wrote:

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

Sounds reasonable. For broader use, I would include the expected value, not
just expected_expr:

elog(ERROR, \
"%s yielded %u, expected %s (%u) in file \"%s\" line %u", \
#result_expr, result, #expected_expr, expected, __FILE__, __LINE__); \

I didn't do that for the atomics tests, where expected_expr is always trivial.
The codebase has plenty of Assert(x == y) where either of x or y could have
the surprising value.

I've been meaning to propose some JUnit-style more-specific Assert
variants such as AssertEquals for this reason. But as Tom writes
nearby, it should be a straight wrapper around Assert, not elog. So
these need to be named separately.

Btw., JUnit uses the ordering convention assertEquals(expected, actual),
whereas Perl Test::More uses is(actual, expected). Let's make sure we
pick something and stick with it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-10-07 19:57, Tom Lane wrote:

I'd just put them all in c.h. I see no reason why a new header
is helpful.

Assert stuff is already in there, but surely stuff that calls elog()
doesn't belong in there?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2019-10-07 13:57:41 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2019-10-05 17:08:38 +0000, Noah Misch wrote:

Report test_atomic_ops() failures consistently, via macros.

I wonder if we should put these (and a few more, for other types), into
a more general place. I would like to have them for writing both tests
like regress.c:test_atomic_ops(), and for writing assertions that
actually display useful error messages. For the former it makes sense
to ERROR out, for the latter they ought to abort, as currently.

IMO, anything named like "assert" ought to act like Assert does now,
ie (1) it's a no-op in a non-assert build and (2) you get an abort()
on failure.

No disagreement at all.

No strong opinions about what the test-and-elog variant
should be called -- but it seems like we might have some difficulty
agreeing on what the appropriate error level is for that. If it's
morally like an Assert except we want it on all the time, should
it be PANIC?

Perhaps it ought to just take elevel as a parameter? Could even be
useful for debugging...

What will happen in frontend code?

Hm. Map to pg_log_*, and abort() if it's an erroring elevel?

Seems like putting ASSERT_{EQ,LT,...}_{U32,S32,...} (or Assert_Eq_...,
but that'd imo look weirder than the inconsistency) into c.h would make
sense, and EXPECT_ somewhere in common/pg_test.h or such?

I'd just put them all in c.h. I see no reason why a new header
is helpful.

WFM.

Greetings,

Andres Freund

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-10-07 19:57, Tom Lane wrote:

I'd just put them all in c.h. I see no reason why a new header
is helpful.

Assert stuff is already in there, but surely stuff that calls elog()
doesn't belong in there?

True, though I had the impression that Andres wanted to propose things
that would work in either frontend or backend, presumably with different
implementations. You could argue it either way as to whether to have
that in c.h (with an #ifdef) or separately in postgres.h and
postgres_fe.h.

regards, tom lane

Hi,

On 2019-10-07 21:58:08 +0200, Peter Eisentraut wrote:

On 2019-10-07 19:57, Tom Lane wrote:

I'd just put them all in c.h. I see no reason why a new header
is helpful.

Assert stuff is already in there, but surely stuff that calls elog()
doesn't belong in there?

Make it call an ExpectFailure() (or similar) function that takes the
various parameters (expression, file, line, severity, format string,
args) as an argument? And that's implemented in terms of elog() in the
backend, and pg_log* + abort() (when appropriate) in the frontend?

Greetings,

Andres Freund

On Mon, Oct 07, 2019 at 09:56:20PM +0200, Peter Eisentraut wrote:

On 2019-10-06 04:20, Noah Misch wrote:

elog(ERROR, \
"%s yielded %u, expected %s (%u) in file \"%s\" line %u", \
#result_expr, result, #expected_expr, expected, __FILE__, __LINE__); \

I've been meaning to propose some JUnit-style more-specific Assert
variants such as AssertEquals for this reason. But as Tom writes
nearby, it should be a straight wrapper around Assert, not elog. So
these need to be named separately.

Agreed.

Btw., JUnit uses the ordering convention assertEquals(expected, actual),
whereas Perl Test::More uses is(actual, expected). Let's make sure we
pick something and stick with it.

Since we write "if (actual == expected)", I prefer f(actual, expected). CUnit
uses CU_ASSERT_EQUAL(actual, expected).

On 2019-10-08 06:59, Noah Misch wrote:

Btw., JUnit uses the ordering convention assertEquals(expected, actual),
whereas Perl Test::More uses is(actual, expected). Let's make sure we
pick something and stick with it.

Since we write "if (actual == expected)", I prefer f(actual, expected). CUnit
uses CU_ASSERT_EQUAL(actual, expected).

Yes, that seems to be the dominating order outside of JUnit.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services