Improve dtest mismatch handling, TABLE compare, and progress UX

[tameware]

Summary

• Stop dtest on the first API fault or expected-result mismatch and exit non-zero instead of always 0.
• Make compare_TABLE verify all DDS_STRAINS, including NT.
• Show batch progress on a single ANSI-overwritten line and clear it when the run finishes.
• Show batch progress on -s calc runs, including GIB-produced solXXX.txt files.

Test plan

• bazelisk test //library/tests:loop_par_test //library/tests:compare_test //library/tests:test_timer_test
• bazelisk run //library/tests:dtest -- -f hands/list100.txt -s calc succeeds and leaves no leftover 100% progress row above the timer summary
• Force a TABLE/PAR mismatch and confirm dtest exits non-zero after reporting only the first difference

Addresses Issue #334

Made with Cursor

[Copilot]

🟢 Approval recommended

The remaining requested integration coverage is a non-blocking nit.

Pull request overview

Improves dtest failure handling, complete TABLE comparisons, and terminal progress output.

Changes:

• Propagates failures as non-zero exits.
• Compares all five strains, including NT.
• Adds progress rendering and cleanup tests.

File summaries

File	Description
library/tests/TestTimer.hpp	Adds progress-line state and cleanup API.
library/tests/TestTimer.cpp	Implements ANSI progress rendering and clearing.
library/tests/testcommon.cpp	Propagates failure statuses.
library/tests/test_timer_test.cpp	Tests progress behavior.
library/tests/loop.hpp	Updates loop return contracts.
library/tests/loop.cpp	Stops on errors or mismatches.
library/tests/loop_par_test.cpp	Tests fail-fast mismatch handling.
library/tests/dtest.cpp	Returns the process status.
library/tests/compare.cpp	Compares all five strains.
library/tests/compare_test.cpp	Tests NT and suit comparisons.
library/tests/BUILD.bazel	Registers the comparison test target.

Review details

Suppressed comments (1)

library/tests/testcommon.cpp:197

• The new exit-status propagation is not covered end to end: loop_par_test only checks the helper's boolean, while the existing dtest integration test covers a successful run. Please add a failing dtest invocation with an intentionally wrong TABLE/PAR result and assert a non-zero process status (and only the first mismatch report), so regressions in the real_main/main wiring at this change are detected.

  return ok ? 0 : 1;

• Files reviewed: 11/11 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Resolve timer finalization on failure paths and provide safe non-TTY progress output.

Review details

Suppressed comments (5)

library/tests/TestTimer.cpp:132

• This now emits VT control bytes unconditionally. When stdout is redirected (for example into CI logs) the escape sequences and carriage returns are recorded literally, and Windows consoles without virtual-terminal processing render them instead of clearing the line. Gate the in-place mode on a TTY/VT-capable stream and keep a newline-based fallback for other outputs.

  cout << "\033[2K\r" << setw(8) << reached << " (" <<

library/tests/loop.cpp:216

• This return bypasses the only timer.end() for the whole PAR batch (line 230). Because real_main now continues to timer.print_hands after a false result, an API fault after any work leaves the batch open and reports zero completed hands; close the timing scope with an accurate processed count before returning (or restructure the timing scope).

        return false;

library/tests/loop.cpp:228

• This mismatch return also skips the only timer.end() for the PAR run (line 230). The new non-exiting path therefore prints a timer summary with zero hands even when earlier iterations were processed; record the elapsed work with an accurate count before returning, or move the timing boundary so failure paths are closed.

    return false;

library/tests/loop.cpp:265

• This return bypasses the only timer.end() for the whole dealer-PAR batch (line 279). Since real_main now proceeds to timer.print_hands after a false result, an API fault after work has been done leaves the timing batch open and reports zero hands; close it with an accurate processed count before returning or restructure the timing scope.

        return false;

library/tests/loop.cpp:277

• This mismatch return also skips the only timer.end() for the dealer-PAR run (line 279). The new non-exiting path can therefore print a zero-hand timer summary after earlier iterations were processed; record the elapsed work with an accurate count before returning, or move the timing boundary so failure paths are closed.

    return false;

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Add failure-path coverage for the remaining loop modes, including calc/TABLE mismatches.

Review details

Suppressed comments (2)

library/tests/loop.cpp:93

• The new loop-level regression coverage only exercises loop_par; the return/cleanup branches added for loop_solve, loop_calc, loop_dealerpar, and loop_play are not executed by either new test. Those paths differ in batching and in whether a progress line is already active, so a regression could still make one of those modes continue after the first mismatch/API fault or leave stale progress while all current tests pass. Please add focused failure tests for the remaining loops (or a shared parameterized equivalent).

      timer.finish_running();
      report_dds_error("loop_solve", ret);
      cout << "loop_solve: i " << i << "\n";
      return false;

library/tests/loop.hpp:40

• The new failure-return contract is applied to all five loops, but the added regression coverage exercises only loop_par. loop_solve, loop_calc, loop_dealerpar, and loop_play still have no tests for an API fault or an early expected-result mismatch, so those paths can regress without detecting the promised non-zero dtest behavior. Add focused failure-path tests for the remaining loops (including a calc/TABLE mismatch case).

/// @return false on DDS API fault or first expected-result mismatch
auto loop_solve(
    BoardsPBN * bop,
    SolvedBoards * solvedbdp,
    DealPBN * deal_list,
    FutureTricks * fut_list,
    const int number,
    const int stepsize,
    std::vector<std::pair<int, int>>* board_times = nullptr) -> bool;

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Two moderate findings remain, and broader loop failure coverage is also needed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

library/tests/TestTimer.cpp:136

• This now emits ANSI control sequences unconditionally, including when stdout is redirected to CI/log files or on native Windows consoles without virtual-terminal processing. Those consumers will see literal \033[2K/carriage returns and a single concatenated progress line instead of readable progress. Please gate the in-place path on a TTY/ANSI-capable stream and retain a newline fallback (or enable VT on Windows).

  cout << "\033[2K\r" << setw(8) << reached << " (" <<
    setw(6) << setprecision(1) << right << fixed <<
      100. * reached /
        static_cast<float>(divisor) << "%)" <<
    setw(15) << right << fixed << setprecision(0) <<

library/tests/testcommon.cpp:197

• The added integration test only exercises an expected PAR mismatch; no test drives a DDS call to return a non-RETURN_NO_FAULT code. Because this line now promises the same non-zero propagation for API faults, add a regression fixture (for example, an overflowing PAR table) and assert the CLI exits non-zero without processing a later deal.

  return ok ? 0 : 1;

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved issues block approval.

Review details

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved timer/progress cleanup paths and a potential dangling ownership bug remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

library/tests/loop.cpp:90

• This branch returns while the timer.start(count) at line 77 is still open. Since real_main now continues to timer.print_hands after a failure, the failed batch is omitted from the timing summary and the timer remains logically open until the next reset. End the timer before clearing/reporting the error, as the PAR loops do.

      timer.finish_running();

library/tests/loop.cpp:172

• This branch returns while the timer.start(count) at line 165 is still open. real_main now prints the timer summary after this return, so an API-fault batch is not recorded and the timing scope is left unfinished. Close the started batch before reporting the error, consistently with the PAR loops.

      timer.finish_running();

library/tests/loop.cpp:364

• This branch returns while the timer.start(count) at line 347 is still open. With the new non-zero-return path, real_main still prints timing data, but this failed batch is omitted and the timer scope is left unfinished. Call timer.end() before clearing/reporting the error.

      timer.finish_running();

library/tests/loop.cpp:285

• This has the same stale-progress problem as the PAR loop: after an earlier successful deal, print_running has activated the line, but this API-fault branch never calls finish_running. The error output can therefore be written after the old progress text instead of replacing it cleanly; clear the line before reporting the fault.

        timer.end();

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two unresolved moderate findings remain regarding large-input parallelism and moved-from temporary cleanup.

Review details

Suppressed comments (2)

library/tests/loop.cpp:155

• This turns the unbounded X API back into repeated MAXNOOFBOARDS-sized jobs. The repository's LargeBatchIsSingleParallelJob test documents that CalcAllTablesX/CalcAllTablesPBNX must dispatch a larger input as one job (library/tests/system/calc_all_tables_x_test.cpp:204-230); with sol100000.txt this loop now performs about 500 conversions, allocations, and scheduler launches instead of one. Please provide progress without forfeiting that whole-input parallelism, or otherwise measure and justify this regression before adopting the fixed batch size.

  for (int i = 0; i < number; i += batch)
  {
    const int count = (i + batch > number ? number - i : batch);

library/tests/loop_failure_test.cpp:79

• The moved-from object's path is left in an unspecified state after std::move. Its destructor still calls release(), which removes path whenever it is non-empty, so a move that leaves the source string populated can delete the same temporary file now owned by the returned object. Clear the source path along with the raw pointers before the moved-from destructor runs.

    path = std::move(other.path);

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[tameware]

library/tests/loop_failure_test.cpp:79 fixed in 38f710f.

[tameware]

library/tests/loop.cpp:155

This turns the unbounded X API back into repeated MAXNOOFBOARDS-sized jobs. The repository's LargeBatchIsSingleParallelJob test documents that CalcAllTablesX/CalcAllTablesPBNX must dispatch a larger input as one job (library/tests/system/calc_all_tables_x_test.cpp:204-230); with sol100000.txt this loop now performs about 500 conversions, allocations, and scheduler launches instead of one. Please provide progress without forfeiting that whole-input parallelism, or otherwise measure and justify this regression before adopting the fixed batch size.

This is not a regression. Batches of 200 versus unbounded batch size have little effect on performance, and batching is the only good way to show progress. Also, dtest is not performance-critical.

bazel-bin/library/tests/dtest -f hands/list1000.txt -s calc

With batching:
User time (ms) 6304

Without batching:
User time (ms) 6192

2% difference.

[Copilot]

🔵 Needs a closer look

loop.cpp caps CalcAllTablesPBNX calls at MAXNOOFBOARDS, regressing the single-job API contract and large-run performance.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

library/tests/loop.cpp:155

• This now caps every CalcAllTablesPBNX call at MAXNOOFBOARDS, so inputs with more than 200 deals are split into multiple solver jobs. That regresses the X-API contract exercised by library/tests/system/calc_all_tables_x_test.cpp:204-230 and documented in library/src/calc_tables.cpp:612-614, where all deal×strain boards are intentionally dispatched in one job; large solXXX.txt runs will pay repeated conversion, preflight, fan-out, and scheduling overhead. Please preserve the single-job path and obtain progress without chunking the API call, or explicitly revisit that API performance contract if chunking is required for the UX.

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The calc batching change can undermine intended large-batch behavior for larger inputs.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

library/tests/loop.cpp:143

• This changes -s calc from the X API's intended single large-batch dispatch into one CalcAllTablesPBNX job per 200 deals. For inputs larger than MAXNOOFBOARDS, that repeats conversion, allocation, scheduling, and worker setup and forfeits the large-batch performance contract documented in library/src/calc_tables.cpp:612-616 and asserted by library/tests/system/calc_all_tables_x_test.cpp:204-230. Preserve the single X-API call for large inputs, or add a solver-side progress mechanism rather than chunking the workload in the harness.

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[tameware]

Per Cursor:

Copilot is mixing up two layers:

Layer	Contract
X-API (CalcAllTablesPBNX)	One call with N deals → one parallel job
dtest (loop_calc)	May issue many X-API calls of size 200 for progress

Batching in loop_calc does not change the library API. calc_all_tables_x_test still proves the single-job path when you pass a large N.