Clean up unit tests: uniform coverage, then uniform structure#286
Clean up unit tests: uniform coverage, then uniform structure#286Bronek wants to merge 68 commits into
Conversation
🤖 Augment PR SummarySummary: This PR standardizes and expands the unit-test suite in two passes — first coverage, then structure — without touching any Coverage pass: Every Structure pass: All files adopt the shape of Utility decoupling: Helper refactoring: Technical note: Where the current library behaviour is wrong, the tests assert it as-is with comments naming the tracking issue, so fixing the defect causes the assertion to fail rather than silently passing. 🤖 Was this summary useful? React with 👍 or 👎 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Select the witness fixtures' throwing-ness and non-constexpr move via a prop flag enum combined with operator|, instead of opaque integer ranges. The three fixtures share a common value base. A prop_of(int) shim keeps the pfn call sites compiling until they migrate to named flags; the aliases keep the legacy helper_t<0> (throwing value ctor) meanwhile. Also drop the dead CODE_COVERAGE_TEST/_ARGS cache variables. Assisted-by: Claude:claude-opus-4-8
Replace the legacy helper_t<int> selectors with their named prop-flag intent; the value-ctor / runtime-move behaviour the integer ranges bundled in incidentally is dropped (nothing relied on it), retiring the prop_of(int) shim on the optional side. Assisted-by: Claude:claude-opus-4-8
Migrate the legacy helper_t<int> selectors to named prop flags. The M/E/C fixtures keep throw_value (value_or / error_or exercise a throwing int->value conversion through them); only helper_t<40>'s incidental non-constexpr move is dropped. No behavioural change to the tests. Assisted-by: Claude:claude-opus-4-8
Both pfn suites now select fixtures with prop flags, so the transitional integer-selector overload and its legacy sanity asserts have no users; the three fixtures go back to template <prop F>. Assisted-by: Claude:claude-opus-4-8
fn::optional::sum_value() declares no noexcept on any overload, including the decltype(auto) ones that return *this by reference and cannot throw (include/fn/optional.hpp:495-510). Add tripwire static_asserts pinning the current behaviour; they flip to noexcept once the pre-1.0 include/ fix lands. Assisted-by: Claude:claude-opus-4-8
Assert the graded or_else's extension contract (include/fn/optional.hpp:172-177): noexcept holds only for a nothrow-invocable callback returning optional<sum<int>> without widening (verified true), and is false for a throwing or widening callback; a non-invocable argument SFINAE-drops or_else from the overload set. Assisted-by: Claude:claude-opus-4-8
…monads Assert the extension noexcept and invocability constraints of and_then and transform over sum/pack, and of operator&, as they currently hold: the sum/pack noexcept is the conservative borrowed-std spec (GH #254), the sum-case transform lacks a callback constraint (hard error, not SFINAE; include/ finding to be raised), and operator& is unconditionally noexcept even for throwing-copy value types (include/ finding to be raised). Add the missing constexpr twins for the pack and operator& sections. Assisted-by: Claude:claude-fable-5
Assert the extension noexcept of and_then/or_else (precise conditional specs, conservatively false over sum/pack visitor sets, GH #254) and of the sum-case transform/transform_error (no spec at all); assert the invocability and untouched-side-copy constraints, incl. clean SFINAE drops for move-only error/value types where the conjunct exists. The sum-case transform and both transform_error overloads lack constraints pfn spells (include/ findings, to be raised). Give the pack or_else/transform_error cases their first compile-time checks and constexpr twins. Assisted-by: Claude:claude-fable-5
…ator& sum_value/sum_error: not-noexcept tripwires on every overload set (issue #276; sum_error shares the gap), and negative probes for sum_value's by-design absence on void values. Pack-value and_then/transform: extension noexcept and pack-apply constraint probes. operator&: assert the unconditional noexcept incl. a throwing-copy value type where the join cannot keep it (include/ finding, as fn/optional), the operand constraints, and constexpr twins for the same-error shapes. Assisted-by: Claude:claude-fable-5
Assisted-by: Claude:claude-fable-5
Tripwires assert the unconditional noexcept currently promised by the copy and move constructors, invoke/invoke_r/transform across all four value categories, the typed-dispatch internals and operator==/!= (#280), plus the converse under-promise on the value constructors and the as_sum lifts. New finding filed as #281: operator!= drops the two sizeof...>0 conjuncts operator== carries, so against a sum<> operand it reports itself viable and then fails to compile in its own body. Assisted-by: Claude:claude-opus-4-8[1m]
The three widening constructors had no tests at all: add value, constexpr, constraint and noexcept coverage for the const&, && and in_place_type forms, including the superset requirement (widening never narrows) and the source's copyable/movable conjuncts witnessed by CopyOnly, MoveOnly and NonCopyable. Add the explicit value constructor, selected when the argument is constructible but not convertible, witnessed by a type with an explicit copy and implicit move constructor; and transform's extra-arguments path, which invoke already covered. Assisted-by: Claude:claude-opus-4-8[1m]
Tripwires assert the unconditional noexcept currently promised by all eight append overloads - which both construct the new element and relocate every existing one - and by invoke/invoke_r across all four value categories (#280), plus the converse under-promise on the as_pack lifts. get's promise is accurate and asserted as such, as is operator&'s absent one, which unlike optional's and expected's (#279) never over-promises. Constraint coverage for append records two new findings: #283, where an element with no default constructor makes the in_place overload drop out and the deduced-argument overload silently append the tag itself as an element; and and SFINAE-clean on clang, so no portable negative probe can be written yet. Assisted-by: Claude:claude-opus-4-8[1m]
Its bind<> is a thin wrapper that calls the predicate and negates it, so the one use here becomes a direct call to the same inline generic lambda. That removes the file's dependency on the fn-implemented test oracle (static_check.hpp is built on fn::monadic_invocable, making judge and judged share code); the header stays for now, still used by the verb files. Assisted-by: Claude:claude-opus-4-8[1m]
Its requires-clause checks only has_type<T>, never that T is constructible from the arguments, so a bad argument list is reported viable and then fails to compile inside variadic_union - outside the immediate context, beyond SFINAE's reach. The probes assert today's wrong answer and flip when the conjunct lands. Assisted-by: Claude:claude-opus-4-8[1m]
Its four-category invoke grid repeated the one already in "sum basic functionality tests", so only its unique checks move across: a callback whose result is not bool, to witness the deduced return, and the white-box read of the stored alternative. Its lambdas also verified the value rather than returning a bare true, so the surviving grid takes that up. Assisted-by: Claude:claude-opus-4-8[1m]
The harness judged fn with fn: monadic_invocable and is_invocable_r came from the library under test, so a bug in them would sit in both judge and judged and go unseen. It now asks std::is_invocable whether the verb's apply accepts the operand - the same question operator| asks - and includes no libfn header at all. Each verb's apply constrains its own operand to a monadic type, so the separate gate that concept provided is not needed. The value-category sweep stays: inlining it would turn each not_invocable<prvalue, cvalue, clvalue, rvalue, crvalue> into five asserts, across some forty sites in and_then.cpp alone. tests_util drops its include_pfn link, closing the circular test-to-library dependency; the fn test targets now name include_fn themselves rather than inheriting it through the utility they test with. Assisted-by: Claude:claude-opus-4-8[1m]
The noexcept dimension contrasts the member, whose spec is precise and weighs the untouched error's copy as well as the callback, against the verb, which discards that answer and is unconditionally noexcept at every step of the pipeline (#285) - so a throwing callback terminates where the member call would propagate. choice is the exception: its own and_then over-promises too, since it dispatches through sum::invoke (#280), which leaves the same operation with different exception behaviour depending on the monad it is written against. fn::invoke, reaching the same apply, reports noexcept(false) even for a callback that cannot throw - the two entry points are wrong in opposite directions. invocable_and_then gains the disjuncts it never covered: choice, and the graded sum-error paths, where the callback may return a different error type for the operation to widen - which is precisely what distinguishes them from the exact-match disjuncts beside them. Assisted-by: Claude:claude-opus-4-8[1m]
A second user-defined verb, whose apply is honestly potentially-throwing, isolates the machinery's promise from the verb's: operator| reports noexcept anyway (#285), so an exception escaping the verb crosses a noexcept boundary and terminates. Asserting it here, against a verb written for the purpose, keeps the finding where the machinery lives instead of restating it in each of the twelve verb test files - which leaves those free to assert only what differs between them, their own member's conditional spec. Assisted-by: Claude:claude-opus-4-8[1m]
Each verb weighs a different untouched side, and the tests now say which: or_else weighs the copy of the value it leaves alone, transform the copy of the error, so a value or error whose copy can throw leaves the member potentially-throwing even for a callback that cannot. Isolating the callback needs an operand whose untouched side is trivially copyable, and both cases are asserted. Each verb's own apply is unconditionally noexcept and discards that answer (#285; the shared pipeline behind it is pinned once in fn/functor.cpp rather than restated here). choice again differs from its siblings: its own transform over-promises too (#280). Assisted-by: Claude:claude-opus-4-8[1m]
The verbs divide by how their apply is written, and the tests now record which side of the line each falls on. transform_error delegates to a member, so it gets the same treatment as or_else: the member weighs the copy of the value it leaves alone, and says so even for a callback that cannot throw. inspect, inspect_error, recover, fail and filter have no member to delegate to - their apply is the implementation, invoking the callback and, for recover, fail and filter, constructing the result as well. Nothing computes an honest spec for them anywhere, so there is none to discard: they are simply declared noexcept over code that may throw. Fixing that group means computing a spec rather than propagating one, so unlike the delegating verbs it does need the traits stubbed false by #45. value_or takes no callback but still builds its fallback value, and promises nothing about that either. discard is the one verb whose unconditional noexcept is accurate - it invokes nothing and returns void - and is asserted positively rather than as a tripwire. Assisted-by: Claude:claude-opus-4-8[1m]
choice inherits sum's unconditional promise and adds its own: defaulted copy and move constructors over an alternative whose own copy can throw, unconditionally noexcept invoke/invoke_r/transform/and_then, and the same converse under-promise on the value constructors (#280). What makes it worth stating plainly is the contrast with its siblings - fn::optional and fn::expected compute a precise spec for these very operations, so the same monadic call propagates through them and terminates through choice. The members inherited from sum whose promise is accurate, has_value and get_ptr, are asserted as such. Assisted-by: Claude:claude-opus-4-8[1m]
choice publicly inherits sum, so has_value, get_ptr, the destructor and the comparison operators are literally sum's code, and sum.cpp already owns them - the duplication #85 names by example. The destructor and has_value/has_type blocks go, both verbatim clones of sum.cpp's; has_type and has_value remain covered by the constructor blocks that exercise them. Equality keeps a short block rather than none: the operators are free functions taking sum<Ts...> const &, so a choice reaching them at all is a fact about its public base and holds nowhere else in the suite. The sixty-line grid comparing alternative lists, which is sum's, does not survive. Assisted-by: Claude:claude-opus-4-8[1m]
The file walked each of the header's five specializations by hand, repeating the same checks per arity against hardcoded .v0/.v1/.v2 members. The battery is now written once - construction, typed access, both dispatch functions in their void and non-void flavours, which the header specializes separately per size - and a TEMPLATE_TEST_CASE ranges it over the five union types. A trait recovers the alternative pack from the union, so the cases name the unions themselves rather than a parallel list of their contents, and ptr_variadic_union serves as the generic accessor in place of the hardcoded members. It runs folded at compile time and again per alternative at run time, so the constexpr twin and coverage are both satisfied and a failure names the alternative rather than only its union. That is broader than what it replaces - the void overloads and the dispatch arity were not covered per alternative before - in 188 lines rather than 430. A non-copyable alternative keeps its own case: the battery copies its witness into the union, so a type that cannot be copied is constructed in place instead. Assisted-by: Claude:claude-opus-4-8[1m]
make has two overloads, and only the first was tested. The second takes over where braces are not viable at all, which is the one thing the pair is for: deleting a type's initializer_list constructor makes it unbraceable while leaving its int constructor reachable through parentheses, and that is what now separates them. The probes are type-keyed concepts, since a requires-expression written against a concrete type hard-errors on an invalid requirement instead of yielding false. overload is asserted to carry each alternative's own noexcept through to the one overload resolution picks - the accuracy the verbs and sum do not manage. The make cases gain runtime twins, having been static_assert-only and so invisible to coverage, and make's own absent noexcept specifier is recorded: the same converse gap as the as_sum and as_pack lifts. Drops the last direct use of static_check::bind. Assisted-by: Claude:claude-opus-4-8[1m]
is_nothrow_invocable and is_nothrow_invocable_r are exported beside their std counterparts but their value is stubbed false, and nothing tested them. They answer false for a callable that plainly cannot throw, where std answers true - and over pack and sum, where no std counterpart exists and the traits are the only oracle there is, the stub is the whole answer. Everything keyed on them inherits it: fn::invoke reports potentially-throwing whatever it is handed. That is the conservative direction, unlike the verbs, which promise noexcept and terminate (#285) - and it is why the self-implementing half of those cannot compute an honest spec until this lands. Assisted-by: Claude:claude-opus-4-8[1m]
same_kind was asserted two hundred times over; its symmetric twin same_value_kind was asserted nowhere, and nor were same_monadic_type_as, convertible_to_bool, or the convertible_to_expected/optional/choice/unexpected family - none of the seven appeared anywhere in the suite. They are not idle: convertible_to_bool is what filter holds its predicate to, and same_value_kind is what transform_error's result is held to. The pair is now tested against each other, since between them they say what a monadic operation may change: same_kind pins the error and lets the value vary, same_value_kind pins the value and lets the error vary, and same_monadic_type_as is the conjunction. The arms that let a sum-valued operand widen, and the one that admits void outright, are covered too. pack_impl gains the noexcept dimension: _swap_invoke is the third of the pipeline's four unconditional boundaries (#285), _append relocates every element while promising not to throw (#280), and _get's promise is accurate. Assisted-by: Claude:claude-opus-4-8[1m]
Take #295's corrected pack.append expectations into the restructured constraints section: it fixes #282 and #283, which this branch had pinned as GAP comments, and makes the sum negative probes portable at last. The #280 noexcept tripwires stay - that issue is still open. Assisted-by: Claude:claude-opus-4-8[1m]
Take #296's corrected expectations for sum, choice and pack. The in_place_type constraints flip #284's tripwires, and the initializability conjunct brings probes this suite could not state before: aggregate brace-init, narrowing rejection, and reference elements bound rather than brace-initialized. Main's new blocks land as SECTIONs, keeping the suite free of the BDD macros. The #280 noexcept tripwires stay - that issue is still open. Assisted-by: Claude:claude-opus-4-8[1m]
Take #297's new constraint sections for the sum-case transform and transform_error, and retire the GAP notes they answer. Both defects were hard errors, so the suite could only describe them in comments: a callback no alternative accepts now drops the candidate, a visitor need only serve the category the call selects, and a move-only value drops the copying overloads. Main's blocks land as SECTIONs, keeping the suite free of the BDD macros. Assisted-by: Claude:claude-opus-4-8[1m]
Flip #276's tripwires: the self-returning sum_value and sum_error overloads are noexcept now, while the lifting ones stay conservatively false and are re-pinned to #280, the sum value constructor whose missing specifier they weigh. Take #298's constexpr coverage, and its free-function noexcept probes folded into the sections that already assert those functions' return types - main states the rest in separate blocks, which those sections already cover, per value category and in place. Assisted-by: Claude:claude-opus-4-8[1m]
Retire #281's tripwires: a rewritten != inherits =='s constraints, so the two now drop out together against the uninstantiable sum<>. The probes move to the equality section, where #299 puts them and where they belong - the noexcept section keeps only what it weighs - and carry both operand orders from #299 plus the empty-on-empty pair the tripwires had. Assisted-by: Claude:claude-opus-4-8[1m]
The GAP note it replaces described an overload that no longer exists: #295 dropped `_append<sum> = delete` as unreachable, and made naming append_type<sum> a clean substitution failure on both compilers, which the note said no portable probe could ask. Assisted-by: Claude:claude-opus-4-8[1m]
Flip #280 and #45: the dispatch machinery, the constructors, append and operator== now weigh what they invoke, construct, relocate and compare, so 66 tripwires across sum, pack, choice, expected, optional and the detail layer reverse - in both directions, since the constructors under-promised where the dispatch over-promised. Each flip gains the converse witness that proves the specification conditional rather than merely false. Two subtleties the tripwires had to be read for, not swept. A pack element relocated from a non-const lvalue selects helper_t's noexcept constructor, so append is noexcept there and throwing in the other categories - the promise tracks the constructor selected. And as_pack still asks is_nothrow_constructible_v where it brace-initializes, an under-promise for every non-empty pack: pinned to #291, which fixes it. Assisted-by: Claude:claude-opus-4-8[1m]
Flip #285: the pipeline propagates now, so 31 tripwires reverse across the verbs, the functor and the nielbloid's own store of the callable. The self-implementing verbs gain the converse witness their apply had no spec to give before, and discard keeps its unconditional promise, which was always the accurate one. Fold #301's per-verb noexcept cases into the per-monad ones this suite already has: they restated the member and the pipe where those are asserted per value category, and four of them collided outright with cases of the same name and tags. Their pipeline assertions survive in place, as does each verb-specific witness - what recover constructs, what filter's two callbacks each contribute, what value_or's fallback costs. Two positive controls had to change fixture: fail and value_or reach an Error carrying a std::string, so their apply is potentially-throwing however nothrow the callback. An error that cannot throw shows the callback deciding alone. Assisted-by: Claude:claude-opus-4-8[1m]
Flip #254 and #279: the sum and pack dispatch answers for its own noexcept now, so the pins that read conservatively false - a visitor set weighed as if std would call it on the sum itself - become true, and operator& promises only what relocating the operands promises, so the throwing-copy witnesses become false. The pack-level operator&, which had no specifier and so never over-promised, gains one too. Take #302's join coverage in place: the value and error relocations per value category, the void operand, the widening error, and the unit-error grade the README leads with - which cannot fail, and now says so. Assisted-by: Claude:claude-opus-4-8[1m]
Take #303's relocation constraints and #304's sum assignment - both additive, so the reconciliation is placement rather than flips: the constraints cases join the verb files, the assignment cases sum and choice, and expected gains the graded shape that could not be assigned at all before. One assertion of ours had to go the other way. #303 asks what value_or actually relocates, and it is the value alone - the error is discarded, never carried into the result - so its throwing copy no longer weighs, and the comment claiming it did described the old imprecision as if it were the design. Assisted-by: Claude:claude-opus-4-8[1m]
sum: the temporary arm of _reinit was entered only on its throwing path, so building the temporary, destroying the old alternative and rebuilding from its move had no runtime coverage - the copy that does not throw now runs it to completion. choice: the heterogeneous comparison answers false where the alternative held is absent from the other list, an arm no test reached; and its comparisons across differing lists were asserted only in constant evaluation, which runs no instrumented code. Assisted-by: Claude:claude-opus-4-8[1m]
e72db1f to
fc2d8c6
Compare
Retire #291's tripwire: as_pack asks the braces it performs, so it no longer under-promises for every argument list, and #305's own section says it better than the pin did - an lvalue binds rather than moves, and the constraint follows the same initialization. Drop our coverage of sum's temporary arm completing, which #305 brings with a constant-evaluation twin: the same test, said twice, would not have compiled. Assisted-by: Claude:claude-opus-4-8[1m]
Retire #290's pin: a void-returning callback now cleanly fails invocable_transform_error, so the negative probe the GAP comment promised is real - spelled fn_Error<void> beside its namespace-block siblings, with #306's category sweep folded into each TC's is:: block; the optional arm gets the same pair via fn_int<void>. Fold #306's concepts.cpp rows onto ours: keep the void and cv-void rows, drop the five positives our block already states. A matching choice-side probe must wait for the next merge: at this merge point invocable_transform over choice still hard-errors on a void-returning callback - the #294 machinery defect, fixed on main by #307. Assisted-by: Claude:claude-fable-5
The last merge of the fix stack. #307's can_transform batteries land as terminal SECTIONs of their TEST_CASEs; the sum-arm probe folds into the namespace block as fn_generic<void> beside the exhaustiveness rows, since our restructure dissolved the TC it targeted. Deliver the choice probe deferred at #306's merge, now that the machinery answers instead of exploding - and its expected- and optional-sum-arm siblings, completing invocable_transform's void row across every disjunct. Assisted-by: Claude:claude-fable-5
|
augment review |
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
|
augment review |
Scope the CHECK-plus-twin rule to behaviour the program performs - a runtime CHECK of a compile-time fact asserts nothing; CLAUDE.md keeps the load-bearing selection and points there. Assisted-by: Claude:claude-fable-5
|
augment review |
Explain the test hierarchy, the dimensions worth exercising, and how to choose between runtime and compile-time assertions. Assisted-by: Codex:gpt-5
Point agents to the contributor test standards and clarify when changed behaviour needs a test in the commit or PR. Assisted-by: Codex:gpt-5
|
augment review |
|



Brings the test suite to a consistent bar, in two passes: first what every file covers, then what every file looks like. Tests only —
include/is unchanged.Coverage. Every
fntest file now checks four dimensions: a runtime check on each behaviour, a constant-evaluation twin, an exact assertion of the deducednoexcept, andrequires-clause accuracy with both positive and negative SFINAE. This addresses the three problems the issue names:Under-testing: surface that had no tests at all is now covered —
sum's widening constructors,make's parenthesis fallback overload,transform's extra-arguments path, seven exported concepts that appeared nowhere in the suite, direct probes of every verb'sinvocable_*concept, and pointer-to-member callables for the verbs that never fed one through the pipe.Over-testing: duplication was removed only where it could be shown mechanically, and each removal was agreed explicitly — a test case whose grid was a verbatim clone of another's, the surface
choicere-tested despite inheriting it fromsum, asum_forblock repeated line-for-line in an unrelated case, and a catch-allTEST_CASE("sum")dissolved into named sections so that its unique content (index,select_nth,get_ptr) lives where a reader would look for it.Inconsistent coverage: the
noexceptandrequiresdimensions, previously near-absent outside the polyfill suites, now apply uniformly across the monads,sum/pack/choice, and all eleven functors.Structure. Every file now follows the shape of
tests/pfn/expected.cpp: plain nestedSECTIONs forming a product of test dimensions, with short names. The BDD macros are gone, and the free-standingconstexpr <functor> <monad>test cases are folded into their monad's case as nested sections.TEMPLATE_TEST_CASEis used only where one battery genuinely serves several subjects:variadic_union(one battery over the header's five specializations, at less than half the code),and_then's member-callable battery (theoptionalhalf was a line-for-line type substitution of theexpectedhalf), and the pack/sum join algebra — the twelve shapes shared bydetail::_joinand the publicoperator&now run through both layers, in both constant and runtime evaluation, which also closed the old_joingrid's complete absence of runtime assertions. Every restructuring step was verified by comparing per-file assertion counts before and after, on both GCC and Clang.One change to the test utilities underpins both passes:
util/static_check.hppno longer uses the library it is used to test. It was built onfn::monadic_invocable, so a bug infnwould have sat in both judge and judged; it now asksstd::is_invocablethe same questionoperator|asks, and includes no libfn header — which also closes the long-standing circular dependency from the test utilities back toinclude/fn.The
noexceptandrequireswork was treated as a discovery tool rather than a formality, and it surfaced a number of genuine defects ininclude/— the concept probes added one more. Each was reproduced on both GCC and Clang and filed separately; none is fixed here. Where the current behaviour is wrong, the tests assert it as it stands, so the assertions fail when the library is fixed rather than silently passing — the sites carry comments naming the issue they belong to. CI's wider matrix caught a handful of portability mistakes in the new tests themselves (old Clang, AppleClang 15, MSVC); all are fixed, each pinned by a comment at the site that bit.Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5