fix: fix ut for sparse builder dump time

[richyreachy]

fix ut for sparse builder dump time

Greptile Summary

This PR disables a flaky unit test assertion (ASSERT_GT(stats.dumped_costtime(), 0UL)) in TestHalfFloatConverter and adds a missing newline at EOF. The dump-time assertion was likely failing intermittently on machines where the dump completes faster than the timer resolution allows.

Key changes:

• The ASSERT_GT(stats.dumped_costtime(), 0UL) assertion is commented out rather than removed, leaving dead commented-out code in the test suite with no explanation of why it was disabled.
• A missing newline at end-of-file is added (minor, but correct).
• No replacement assertion or explanatory comment is provided, so the intent behind the original check is completely lost.

Confidence Score: 3/5

• Safe to merge functionally, but the commented-out assertion reduces test quality without explanation.
• The change only affects a test file and will not break production behavior. However, replacing a meaningful assertion with commented-out dead code (rather than removing it cleanly or substituting a better assertion) lowers the score — it leaves the codebase in a slightly worse state than simply deleting the line would.
• tests/core/algorithm/flat_sparse/flat_sparse_builder_test.cc — the commented-out assertion at line 260 should be removed or replaced with an explanatory comment.

Important Files Changed

Filename	Overview
tests/core/algorithm/flat_sparse/flat_sparse_builder_test.cc	Assertion ASSERT_GT(stats.dumped_costtime(), 0UL) is commented out instead of being properly removed, and a missing newline at EOF is fixed. The commented-out assertion weakens test coverage without explanation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[TestHalfFloatConverter] --> B[Create & init builder]
    B --> C[Train on converted_holder]
    C --> D[Build on converted_holder]
    D --> E[Dump via FileDumper]
    E --> F[Assert stats counts]
    F --> G[Assert trained_costtime == 0]
    G --> H[Assert built_costtime == 0]
    H --> I{Assert dumped_costtime > 0}
    I -- "REMOVED in this PR (commented out)" --> J[⚠️ No coverage for dump time]
    I -- "Expected: records elapsed time" --> K[✅ Validate dump timing]
    J --> L[Cleanup & rebuild with holder2]
    L --> M[Dump again via FileDumper2]
    M --> N[Assert stats counts for rebuild]
    N --> O[No dumped_costtime assertion in rebuild section either]

Loading

Comments Outside Diff (1)

1. tests/core/algorithm/flat_sparse/flat_sparse_builder_test.cc, line 291-296 (link)

   Missing dumped_costtime assertion in second dump cycle

   The first dump cycle (around line 250) originally verified that stats.dumped_costtime() > 0. The second rebuild-and-dump cycle (starting at line 285) never had any assertion for dumped_costtime() at all. If the concern about timer resolution applies to the first dump, it equally applies here — but it also means this second cycle has never had any coverage of the dump timing stat. If dumped_costtime is worth asserting, it should be consistent across both dump cycles; if it's not reliable enough to assert, a comment explaining why (e.g., "timer resolution too coarse on fast CI machines") would help readers understand the omission.

_{Last reviewed commit: f5880b7}

[richyreachy]

@greptile

[richyreachy]

@greptile

[richyreachy]

@greptile