Skip to content

test: reorganize C++/C#/Go test suites into per-feature files (load/get/ordered_map/index)#166

Merged
wenchy merged 2 commits into
masterfrom
multilang-test-split
Jun 15, 2026
Merged

test: reorganize C++/C#/Go test suites into per-feature files (load/get/ordered_map/index)#166
wenchy merged 2 commits into
masterfrom
multilang-test-split

Conversation

@Kybxd

@Kybxd Kybxd commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Reorganizes the C++, C# and Go test suites from a few monolithic files into
focused, per-feature test files, mirroring the four functional areas of the
tableau loader: Load → Get → OrderedMap → Index. This brings the three
native-language suites in line with the structure already adopted for TS.

Note: TS-side changes live in a separate branch (ts-loader); this PR is
intentionally scoped to C++, C# and Go only and branches directly off master.

What changed

C++ (GoogleTest)

  • Split hub_test.cpp / patch_test.cpp into:
    • load_test.cpp, get_test.cpp, ordered_map_test.cpp, index_test.cpp
  • Extracted the shared HubFixture into a new header tests/hub_fixture.h
    so every test file reuses one initialized Hub instead of duplicating setup.
  • test_paths.h: minor path addition to support the new files.

C# (xUnit)

  • Replaced ActivityConfTests.cs / BinTests.cs / HubTests.cs / PatchTests.cs
    with feature-grouped files: LoadTests.cs, GetTests.cs, OrderedMapTests.cs,
    IndexTests.cs (shared HubFixture / HubCollection retained).

Go

  • Split main_test.go into load_test.go, get_test.go, ordered_map_test.go
    and reworked index_test.go; shared setup via the prepareHub(t) helper.

Generated sources

  • Refreshed the generated protoconf loader sources (C++ *.pc.cc/.pc.h,
    Go *.pc.go) — header/comment-level regeneration only, no behavioral change.

Verification

make.py test passes for all three languages via the full
codegen → build → test pipeline:

Lang Flow Result
C++ clean → buf generate → cmake → build → ctest 26/26 passed
C# buf generate → dotnet test 25 passed
Go buf generate → go test -race ./... ok

Notes

  • Pure test reorganization + generated-code refresh; no production/runtime code
    is touched.
  • File renames are detected by Git (patch_test.cpp → load_test.cpp,
    PatchTests.cs → LoadTests.cs, main_test.go → load_test.go) to preserve history.

Reorganize the C++, C# and Go test suites into per-feature files (load/get/ordered_map/index), extract shared setup (C++ hub_fixture.h), and refresh the generated protoconf loader sources accordingly.
@Kybxd Kybxd requested a review from wenchy June 15, 2026 03:05

@wenchy wenchy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placeholder

@wenchy

wenchy commented Jun 15, 2026

Copy link
Copy Markdown
Member

Code Review findings (10 items, ranked by severity)

See full details in the separate review comment. Summary:

Red (correctness/CI)

  1. make.py:974_prepend_path skips GITHUB_PATH when dir already on PATH (fix: move the GITHUB_PATH append before the early-return)
  2. ModeOnlyPatch tests assert name != '' which passes even if mode is silently ignored — assert the specific expected value 'orange' instead (all 3 languages)

Yellow (coverage/release)
3. ordered_map_test.cpp — lost direct coverage of Hub::GetOrderedMap<Mgr,T>(leveled-keys); the deleted hub_test.cpp tested both 1-key and 3-key overloads, new file only uses no-arg conf->GetOrderedMap()
4. cmd/protoc-gen-csharp-tableau-loader/main.go:13 — version still 0.1.0 while go+cpp bumped to 0.12.0
5. IndexTests.csFindFirstAwardItem not called (C++ test at index_test.cpp:133 does assert it)

Green (minor)
6. BUF_VERSION dead in workflow env after buf-action removal
7. 2nd-level sub-map non-empty assertion dropped from C++/C# ordered-map tests
8. Redundant bool first = true guard in 3 mirrored ordered-map test files (C++/C#/Go) — prev=0 already handles iter-1 correctly
9. FruitType constants duplicated in C# GetTests.cs + IndexTests.cs; C++ and Go centralize them
10. LoadTests.cs:PatchConf2_DifferentFormat_PatchPathsOverride name claims txtpb but test uses .json (comment explains C# does not support txtpb)

@wenchy

wenchy commented Jun 15, 2026

Copy link
Copy Markdown
Member

Details for Finding 1 — make.py:974 GITHUB_PATH skip

Root cause: _prepend_path early-returns when the dir is already in os.environ['PATH'], before the GITHUB_PATH append at line 979. ubuntu-latest runners have ~/.local/bin on PATH by default, so the persistence block is never reached when buf is installed there. The next workflow step's fresh shell (rebuilt from GITHUB_PATH) loses buf.

Fix: move the GITHUB_PATH write above the membership check, so it fires unconditionally regardless of in-process PATH state.

@wenchy

wenchy commented Jun 15, 2026

Copy link
Copy Markdown
Member

Details for Finding 2 — ModeOnlyPatch assertions too weak (all 3 languages)

conf/PatchMergeConf.json sets name='apple'; patchconf/PatchMergeConf.json sets name='orange'. The tests assert only that name is non-empty, which passes whether ModeOnlyPatch works correctly (yields 'orange') or is silently a no-op (yields 'orange' from main+patch merge, or 'apple' from main-only — both non-empty).

Fix: assert name equals 'orange' specifically. This proves the patch value was applied AND the main file's value was not the source.

@wenchy

wenchy commented Jun 15, 2026

Copy link
Copy Markdown
Member

Details for Finding 3 — lost Hub::GetOrderedMap<Mgr,T>(leveled-keys) coverage in C++

The deleted hub_test.cpp contained:

  • ActivityConf_GetOrderedMap_Chapter: called Hub::Instance().GetOrderedMap<ActivityConfMgr, OrderedMap_Activity_ChapterMap>(100001) and asserted non-null + non-empty
  • ActivityConf_GetOrderedMap_Rank: called Hub::Instance().GetOrderedMap<ActivityConfMgr, OrderedMap_int32Map>(100001, 1, 2) and asserted non-null

The new ordered_map_test.cpp only calls conf->GetOrderedMap() (no-arg messager method) and walks deeper via iterator — the Hub API's multi-key template overload is never invoked anywhere in the test suite. Grep across test/cpp-tableau-loader/tests/ confirms zero remaining calls to Hub::Instance().GetOrderedMap with arguments.

Fix: add a test to ordered_map_test.cpp that calls the 1-key overload (chapter sub-map) and the 3-key overload (rank map), asserting each is non-null.

@wenchy

wenchy commented Jun 15, 2026

Copy link
Copy Markdown
Member

Details for Findings 4–10 (yellow/green)

Finding 4 — csharp version not bumped (cmd/protoc-gen-csharp-tableau-loader/main.go:13)
go and cpp both went 0.11.0 -> 0.12.0; csharp is still at 0.1.0. The version constant is stamped into every generated *.pc.cs, so generated C# output will display v0.1.0 while go/cpp display v0.12.0 in the same release cycle. Please bump or confirm csharp uses an independent cadence.

Finding 5 — IndexTests.cs missing FindFirstAwardItem
index_test.cpp:133 asserts ASSERT_NE(item->FindFirstAwardItem(1, 'apple'), nullptr). The C# peer tests FindAwardItem and FindAwardItemMap but never calls FindFirstAwardItem. Add Assert.NotNull(item.FindFirstAwardItem(1, 'apple')) to ItemConf_AwardItem_MultiColumnIndex.

Finding 6 — BUF_VERSION dead in workflow env
.github/actions/load-versions/action.yml still exports BUF_VERSION to GITHUB_ENV; no workflow consumes env.BUF_VERSION after the buf-action removal. make.py reads the version directly from versions.env. Remove or document.

Finding 7 — 2nd-level sub-map non-empty assertion dropped
Old hub_test.cpp asserted EXPECT_FALSE(chapter_ordered_map->empty()). New ordered_map_test.cpp and C# OrderedMapTests silently no-op if the chapter sub-map is empty. Add emptiness assertions inside the inner traversal loop.

Finding 8 — redundant 'first' flag in ordered-map tests (3 languages)
ordered_map_test.cpp:55, OrderedMapTests.cs:53, ordered_map_test.go:56 all use 'bool first = true' to skip the ascending check on iter-1, but prev is initialized to 0/uint.MinValue so key >= prev is always true on iter-1 anyway. The flag is dead state. Compare with the cleaner pattern in index_test.cpp (uses numeric_limits::min, no flag).

Finding 9 — FruitType constants duplicated in C#
GetTests.cs:22 and IndexTests.cs:22-24 each declare private const int FruitTypeApple/Orange/Banana. C++ centralizes these in hub_fixture.h; Go uses package-level vars in index_test.go. Hoist to HubFixture.cs to avoid sync issues.

Finding 10 — LoadTests.cs:PatchConf2_DifferentFormat_PatchPathsOverride misleading name
The test name claims DifferentFormat (txtpb) but the body uses .json with a comment explaining the C# loader does not support txtpb. As written it duplicates the multi-patch scenario. Either rename to remove 'DifferentFormat' or add an explicit skip annotation documenting the format gap.

- make.py: persist GITHUB_PATH even when dir already on in-process PATH,
  so tools installed by 'make.py setup' are visible to later GHA steps
- ModeOnlyPatch: assert concrete name="orange" instead of just non-empty
  (cpp/csharp/go)
- cpp ordered_map_test: restore leveled-keys GetOrderedMap coverage
  (1-key chapter map, 3-key rank map)
- csharp IndexTests: add FindFirstAwardItem assertion to mirror cpp
- cpp/csharp ordered_map: assert 2nd-level sub-map is non-empty
- cpp/csharp/go: drop redundant 'first' guard in ascending-key check
  (prev=0 already handles iteration 1)
- csharp: centralize FruitType constants into HubFixture.FruitTypes,
  mirroring cpp (hub_fixture.h) and go (index_test.go)
- csharp LoadTests: rename misleading test to
  PatchConf2_PatchPathsOverride_UsesJson and clarify comment
@Kybxd Kybxd requested a review from wenchy June 15, 2026 08:04
@wenchy wenchy merged commit 8ffb35b into master Jun 15, 2026
10 checks passed
@wenchy wenchy deleted the multilang-test-split branch June 15, 2026 10:56
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