CDRIVER-4537 Ensure all aborts in test suite are preceeded by flushes #1164
Conversation
There was a problem hiding this comment.
Easier to read log output is really helpful.
future.c: avoid includingTestSuite.h(unnecessary concern?)
That might be OK. mock-server.c already includes TestSuite.h. An eventual improvement may be to split the assertions from TestSuite.h into a separate header. But not necessary in this PR.
| #define FUTURE_TIMEOUT_ABORT \ | ||
| if (1) { \ | ||
| fflush (stdout); \ | ||
| fprintf (stderr, "%s timed out\n", BSON_FUNC); \ | ||
| fflush (stderr); \ | ||
| abort (); \ | ||
| } else \ | ||
| ((void) 0) |
There was a problem hiding this comment.
I'm curious why we use this if (1) { ... } else pattern in several places rather than the more common do { ... } while (0)?
There was a problem hiding this comment.
I think that pattern started recently from @vector-of-bool. This article describes the motivation.
| #define ASSERT_OP_TIMES_EQUAL(_a, _b) \ | ||
| if ((_a).t != (_b).t || (_a).i != (_b).i) { \ | ||
| test_error (#_a " (%d, %d) does not match " #_b " (%d, %d)", \ | ||
| (_a).t, \ | ||
| (_a).i, \ | ||
| (_b).t, \ | ||
| (_b).i); \ |
There was a problem hiding this comment.
I would suggest that we wrap multiline macros such as this in a do { ... } while (0), since as written it can behave unexpectedly within an if/else, for example, if we have the following code, we will get a syntax error:
11 #define FOO(X) \
12 if (X) { \
13 foo(X); \
14 bar(X); \
15 }
...
25 int main(int argc, char **argv) {
26 if (argc > 2)
27 FOO(argc);
28 else
29 puts("hello");
30 }
Syntax error:
2022-12-16 9:04:05 [~/src/test]$ gcc dowhile.c
dowhile.c:28:5: error: expected expression
else
^
1 error generated.
However, if we wrap in a do/while(0), the macro works as expected:
17 #define BAR(X) \
18 do { \
19 if (X) { \
20 foo(X); \
21 bar(X); \
22 } \
23 } while (0)
24
25 int main(int argc, char **argv) {
26 if (argc > 2)
27 BAR(argc);
28 else
29 puts("hello");
30 }
Compiles as expected:
2022-12-16 9:04:37 [~/src/test]$ gcc dowhile.c
2022-12-16 9:04:38 [~/src/test]$
There was a problem hiding this comment.
Good spot. Updated accordingly using the pattern described in your other comment.
This PR extends the work done for CDRIVER-4537 by #1161. Verified by this patch.
This PR ensures (almost) all instances of a call to
abort()withinsrc/libmongoc/testsis replaced by an invocation oftest_error ()instead. The only remaining exceptions are:TestSuite.handTestSuite.c: defines assertion macros used by test suite.future.c: avoid includingTestSuite.h(unnecessary concern?)test-mongoc-gssapi.c: its own executable; avoid introducing linkage withTestSuite.c.All
abort ()are replaced by a call totest_error, which handles flushing ofstdoutandstderr. Any immediately preceeding messages emited viafprintf()orMONGOC_ERRORare moved into the call totest_error. Final'\n'characters were removed from error messages astest_errorensures the emitted message is followed by a newline character.This PR is motivated by confusing errors messages in failing tasks such as in this patch:
Synchronization of
stdoutandstderrensures the output is not interspersed withstdoutmessages, as demonstrated by this patch: