chore: sweep the error-event assertions onto the shared macros - #789
Conversation
CHECK_ERROR_EVENT has been in Tests/Support/TestUtils.h since it was written and had no call sites, while 136 blocks across 47 files hand-rolled the same four assertions. #788 gave it the severity axis and a CHECK_ERROR_REPORTED_ONCE sibling and converted five sites; this converts the rest. 122 blocks were the identical shape - count, severity, source, category, detail, in that order - and became CHECK_ERROR_REPORTED_ONCE mechanically. Ten file-local macros wrapped the same block with a fixed source: five copies of CHECK_REPORTED in the lwIP tests, four of CHECK_REPORTED_ERROR in the policy tests, and CHECK_STREAMSENDER_BAD_SETUP_ERROR. Each now delegates to the shared macro rather than restating it, as CHECK_OPEN_UNWOUND_WITH_SEVERITY already does in both stream test files. Two sites assert the second of two events, where pinning the count at ONCE would be wrong. They use CHECK_ERROR_EVENT with their own CALLED_FAKE(..., TWICE) left above it, which is what the two macros were split for. Three sites asserted a subset - one without a source, two without a source or a severity - and were invisible to a survey anchored on the source assertion. They now assert all four axes, with the missing values read from the emit site rather than assumed: BlockStore's WARNING, the POSIX message-queue buffer's ERROR, and CRITICAL for the UDP sender's null-buffer argument check, which its own sibling test already asserted. Those five new checks pass, so no drift was hiding there. One assertion is deliberately left hand-rolled: SolidSyslogErrorTest also checks LastContext(), which no shared macro covers, and it sits after the converted block. The sweep does not shrink the test code - it adds about seventy lines, because a four-argument call wraps where five short assertions did not. What it buys is one assertion shape, a severity axis asserted everywhere rather than in most places, and one place to change if the event gains a field. Tests only; no production source is touched. 1496 unit tests, 3589 checks - five more than before, being the axes the three partial sites had never asserted - and 22 of 22 suites including both integration lanes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe change replaces repeated ChangesError assertion migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR refactors test assertions and test-local setup helpers without changing production behavior; the supplied tests and formatting checks pass, and no actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The file carried two ways to swap the config and rebuild the stream: seven tests hand-rolled Destroy + Create, while their neighbours called ReCreateStreamWithUpdatedConfig. The two are not equivalent - the helper also rebuilds the transport fake and reinstalls the error handler, so counts start from the recreate - and nothing in the file said which to reach for. The seven now call the helper. All seven were the same arrange shape: destroy, set one config field, create. None arranged fake state before recreating, which is what would have changed meaning rather than failing; CipherListFailureFreesCtx forces its fake after the create, which is the order the helper requires and already had. The remaining Destroy calls in test bodies stay. In all seven of those, Destroy is the action under test - freeing the SSL, the BIO method, the context, closing a still-open transport - and one re-creates afterwards so teardown targets a live slot rather than a stale handle. This is scoped to the one file that had both idioms. The hand-rolled form is used in 36 other test files, consistently within each, and converting those would need a helper per fixture and individual judgement at the seven sites tree-wide that do arrange fakes first. 1496 unit tests, 3589 checks, 22 of 22 suites.
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
The follow-up agreed on #788.
CHECK_ERROR_EVENThad sat inTests/Support/TestUtils.hsince it was writtenwith no call sites, while 136 blocks across 47 files hand-rolled the same four
assertions. #788 gave it the severity axis and a
CHECK_ERROR_REPORTED_ONCEsibling and converted five sites; this converts the rest.
Tests only. No production source is touched.
No
Closeskeyword: per #782,intermediate pull requests into
feature/tls-reworkcarry none.Change Description
122 blocks were mechanical. Count, severity, source, category, detail, in
that order - the same order as the macro's parameters. Converted by script, then
clang-format.Ten file-local macros wrapped the same block with a fixed source: five copies
of
CHECK_REPORTEDacross the lwIP tests, four ofCHECK_REPORTED_ERRORacrossthe policy tests, and
CHECK_STREAMSENDER_BAD_SETUP_ERROR. Each now delegates tothe shared macro instead of restating it, which is what
CHECK_OPEN_UNWOUND_WITH_SEVERITYalready did in both stream test files. That isthe part of this worth reviewing: ten near-identical copies were the actual
duplication, not the call sites.
Two sites assert the second of two events, where pinning the count at
ONCEwould be wrong. They use
CHECK_ERROR_EVENTwith their ownCALLED_FAKE(..., TWICE)above it - the case the two macros were split for.Three sites asserted a subset, and were invisible to a survey anchored on the
source assertion: one without a source, two without a source or a severity. They
now assert all four axes, with the missing values read from the emit site rather
than assumed -
BlockStore'sWARNING, the POSIX message-queue buffer'sERROR, andCRITICALfor the UDP sender's null-buffer check, which that file'sown sibling test already asserted. Those five new checks pass, so nothing was
drifting behind the gap.
One assertion stays hand-rolled:
SolidSyslogErrorTestalso checksLastContext(), which no shared macro covers, and it sits directly after theconverted block.
It does not shrink the tests
About seventy lines longer, because a four-argument call wraps where five short
assertions did not. What it buys is one assertion shape, a severity axis asserted
everywhere rather than in most places, and one place to change if the event ever
gains a field. Worth saying plainly, since "DRY sweep" implies a smaller diff than
this produces.
Test Evidence
3589 checks against 3584 before: the five extra are the axes the three partial
sites had never asserted. The 22 includes both integration suites against the real
libraries, and the lwIP, FreeRTOS, FatFs and PlusFAT suites that the
debugpreset in the
gccimage does not build - the ten delegated local macros livemostly in those, so they would otherwise have gone unverified locally.
Whole-tree
clang-format --dry-run --Werrorclean. No MISRA step: no productionsource changed, so no suppression line moved.
Areas Affected
47 files under
Tests/, all test-tier.Tests/Support/TestUtils.his unchanged -#788 already put both macros there.
Windows test files are converted but not exercised locally;
build-windows-msvccovers them.
Summary by CodeRabbit
Second commit: one recreate idiom in the OpenSSL stream tests
Added after review discussion. That file carried two ways to swap the config and
rebuild the stream - seven tests hand-rolled
Destroy+Create, theirneighbours called
ReCreateStreamWithUpdatedConfig()- and the two are notequivalent, since the helper also rebuilds the transport fake and reinstalls the
error handler. Nothing said which to reach for. The seven now call the helper.
The
Destroycalls that remain in test bodies are the seven lifecycle tests whereDestroyis the action under test, plus one that re-creates afterwards soteardown targets a live slot rather than a stale handle.
Deliberately not extended tree-wide. The hand-rolled form appears in 36 other
test files and is used consistently within each, so those files are already
internally consistent; converting them would need a recreate helper per fixture.
More to the point, seven sites across the tree - in
UdpSenderTest,BlockStoreTest,StreamSenderTest,LwipRawTcpStreamTestandSenderFakeTest-arrange fake state before recreating. Pointed at a helper that resets on
recreate, those would not fail; they would pass while no longer testing what they
were written to test. That is the reason this stops at the one file that had both
idioms.