Skip to content

refactor(test): simplify GlobalsGuard and TempDir, and test the harness - #122

Merged
TimD1 merged 2 commits into
devfrom
119_D2_td_harness-cleanups
Aug 3, 2026
Merged

refactor(test): simplify GlobalsGuard and TempDir, and test the harness#122
TimD1 merged 2 commits into
devfrom
119_D2_td_harness-cleanups

Conversation

@TimD1-bot

Copy link
Copy Markdown
Collaborator

Note

Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via gh under @TimD1-bot, a bot account operated by @TimD1. It reflects the
agent's analysis, not a statement authored by @TimD1.

Closes #119. Part of #45; follow-up to #118, which landed the unit-test harness.

Three cleanups to the unit-test scaffolding, done before the per-module test PRs build much on top of it.

1. Globals::VERSION/PROGRAM are now static

VERSION and PROGRAM are compile-time constants identical in every Globals instance, so they were never per-object state — but as non-static const members they deleted the implicitly-generated copy-assignment operator. Making them static const std::string (declared in src/globals.h, defined in src/globals.cpp) restores copy-assignment.

Every existing use goes through .data(), which is unchanged on a static const std::string, so there are no call-site edits: src/globals.cpp (print_version, print_usage), src/print.cpp (write_params), and the WARN/INFO/ERROR macros in src/defs.h.

2. GlobalsGuard no longer enumerates fields

With Globals copy-assignable, the destructor collapses from a 33-field assignment list to g = this->saved;. The public interface is unchanged — GlobalsGuard guard; at the top of a test behaves exactly as before.

The point is that a newly added Globals field can no longer silently escape restoration. Previously it would leak into the next test, and because gtest orders suites across translation units by link order, the resulting failure could surface in an unrelated test file.

3. TempDir drops the vector<char> copy

mkdtemp mutates its argument, which used to require copying the template through a std::vector<char>. Non-const std::string::data() is available in C++17 and the build already uses -std=c++17, so mkdtemp(tmpl.data()) works directly. The _XXXXXX suffix is untouched — mkdtemp requires exactly six trailing X characters.

4. tests/unit/src/test_harness.cpp

The scaffolding itself had no committed test, so a defect in it surfaced inside somebody else's test file. Eleven new cases, one gtest suite per helper:

Suite Case Covers
TempDir CreatesWritable the directory exists and a file can be written into it
TempDir DistinctPaths two live instances get different paths
TempDir RemovesRecursively a directory containing a file is gone after destruction
TempDir PathJoin path(name) is path() + "/" + name, with no doubled separator
GlobalsGuard RestoresScalar a mutated g.sv_threshold is restored at scope exit
GlobalsGuard RestoresContainer same for g.filters, which a field-by-field restore is likeliest to miss
GlobalsGuard SilencesVerbosity g.verbosity == 0 inside the guard's scope
WriteTmpVcf ParsesUnderHtslib the emitted file is accepted by parse_variants, the helper's entire contract
MakeFasta SequenceReadable sequence round-trips, uppercasing is applied, contigs stay independent
MakeCtgVariants Roundtrip count, order, and per-variant fields survive the builder
AllocReachOffs SizeAndInit the buffer is MATS*(max(x,o+e)+1)*(qlen+tlen-1) entries, all -2

test_harness.o is added to OBJS in tests/unit/build/Makefile; the existing pattern rule needs no change.

Verification

  • cd tests/unit/build && make — builds clean (the one warning, dist.cpp:1070 unused parameter thread2, predates this branch).
  • ./test_vcfdist22 tests from 9 test suites ran. / [ PASSED ] 22 tests., up from 11.
  • cd src && make clean && make — links vcfdist; ./vcfdist --version prints vcfdist v3.0.0-b0, exercising both static members through .data().
  • cd src && doxygen Doxyfile 2>&1 | grep -i warning — silent.
  • cd tests && pytest3 passed.

TimD1 added 2 commits August 3, 2026 15:54
Make Globals::VERSION and Globals::PROGRAM static. They are compile-time
constants identical in every instance, so they are not per-object state; as
non-static const members they deleted Globals' copy-assignment operator.
GlobalsGuard's destructor can now restore global state with one whole-struct
assignment instead of enumerating 33 fields, which a newly added Globals
field could silently escape and leak into an unrelated test file. Every
existing use goes through .data(), unchanged on a static const std::string.

Drop the std::vector<char> copy in TempDir: C++17's non-const
std::string::data() lets mkdtemp mutate the template in place.

Add tests/unit/src/test_harness.cpp, covering TempDir creation, uniqueness,
recursive removal and path joining; GlobalsGuard scalar/container restoration
and verbosity silencing; write_tmp_vcf output parsing under htslib; and the
make_fasta, make_ctgVariants and alloc_reach_offs builders. A defect in the
scaffolding now fails in its own file rather than somebody else's.

Closes #119
Closes #128.

add_var() defaults every parameter after phase_set, so make_ctgVariants() passing
var.supercluster in the eleventh position bound it to rec_idx once #116 inserted the three
provenance parameters mid-signature, and supercluster silently took its -1 default. Both
are int, so it compiled clean under -Wall -Wextra.

var_desc gains rec_idx, alt_idx and ploidy fields (defaults -1, -1, 0) and all four are now
passed explicitly, so tests can exercise the new per-variant vectors. The new fields are
appended rather than inserted, so positional brace-initialization of var_desc keeps binding
to the same members.

MakeCtgVariants.ProvenanceFieldsReachTheirOwnVectors gives each of the five trailing fields
a distinct value, so a future positional shift fails a test instead of passing silently.

Also corrects var_desc::qual's doc comment, which claimed add_var() clamps it to g.max_qual:
that holds for var_qual only, not gt_qual (#135). No behavior change in src/variant.cpp.
@TimD1
TimD1 force-pushed the 119_D2_td_harness-cleanups branch from 3ecbb74 to faa9207 Compare August 3, 2026 19:59
@TimD1-bot

Copy link
Copy Markdown
Collaborator Author

Claude Opus 5 🤖: Rebased onto dev and fixed #128 in the same push.

Rebase

No conflicts. dev moved forward by #116 (per-variant provenance vectors) and #127 (push policy); neither touches a file this branch modifies, so the single commit replayed cleanly and nothing needed reconciling.

#128make_ctgVariants fixed

#116 inserted rec_idx, alt_idx and ploidy into the middle of add_var's signature, so make_ctgVariants passing var.supercluster eleventh bound it to rec_idx, and supercluster silently took its -1 default. That is exactly what failed MakeCtgVariants.Roundtrip here.

Took the richer fix #128 suggested rather than the minimal -1, -1, 0 literals: var_desc gains rec_idx = -1, alt_idx = -1 and ploidy = 0, and all four are passed explicitly — so the sibling test PRs can drive the new vectors rather than only work around them.

One deliberate deviation. The new fields are appended after loc, not inserted beside supercluster where add_var's own parameter order would put them. var_desc is aggregate-initialized positionally, so inserting mid-struct is the same silent-rebinding hazard #128 is about: it would quietly reinterpret every brace-initializer in the test PRs currently open. Appending keeps existing initializers bound to the same members, and the struct's doc comment now states the rule.

New regression test

MakeCtgVariants.ProvenanceFieldsReachTheirOwnVectors gives phase_set, rec_idx, alt_idx, ploidy and supercluster five distinct values and asserts each lands in its own vector, then checks that an unspecified var_desc still yields add_var's -1/-1/0/-1 sentinels. Confirmed it pins the defect: reverting only the make_ctgVariants call fails it on ploidies[0] and superclusters[0], alongside Roundtrip.

Also, #135's documentation half

var_desc::qual's comment claimed "add_var clamps to g.max_qual", which holds for var_qual only. It now reads Sets var_qual (clamped to g.max_qual) and gt_qual (unclamped). src/variant.cpp is untouched — whether gt_qual should also be clamped is #135's decision, and it changes published values.

Verification

  • cd tests/unit/build && make — clean; the one warning, dist.cpp:1070 unused parameter thread2, predates this branch.
  • ./test_vcfdist31 tests from 11 test suites ran. / [ PASSED ] 31 tests. Up from 22: feat(variant): add per-variant provenance and ploidy vectors #116 brought 8 ProvenanceVectors* cases in on dev, and this adds 1.
  • ./test_vcfdist --gtest_filter='MakeCtgVariants.*' — both cases pass.
  • cd src && make — links vcfdist; ./vcfdist --version prints vcfdist v3.0.0-b0, exercising both static members.
  • cd tests && pytest3 passed.
  • cd src && doxygen Doxyfile 2>&1 | grep -i warning — silent.

No src/ change beyond what the rebase brought in, so there is no count impact to report.

@TimD1
TimD1 merged commit a7283f0 into dev Aug 3, 2026
1 check passed
@TimD1
TimD1 deleted the 119_D2_td_harness-cleanups branch August 3, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants