new package cl/cltest - #702
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR is clean, well-scoped WIP scaffolding: it renames the embedded clang.TranslationUnit in cl.Source to a named field TU, introduces a Config.NameLookup hook, and extracts the TestFromDir helper into a new reusable cl/cltest package while moving the test to an external cl_test package. Build and go vet pass for ./cl/....
Positives
- Renaming the embedded field to
TUavoids promoting allTranslationUnitmethods ontoSource— a real API-surface improvement. - Moving the helper to
cl/cltestand switching tocl_testexercises the package through its public API. - New exported symbols carry doc comments;
go.mod/go.sumcorrectly reflect the newgithub.com/qiniu/xdependency (test-only).
Notes (non-blocking)
- No security or performance concerns found. The new
github.com/qiniu/xdependency is confined to test code. - The main items are inline:
Config.NameLookupis currently inert, and the only test covering the new path (_TestMockC) is disabled, so the rename/new package have no enforced CI coverage yet. Both are acceptable as scaffolding if a follow-up wires them in.
| *Reused | ||
|
|
||
| // NameLookup looks up the archive path for a given mangling name. It returns the archive path and a boolean indicating whether the lookup was successful. | ||
| NameLookup func(manglingName string) (archivePath string, ok bool) |
There was a problem hiding this comment.
[P2] Config.NameLookup is declared but never consumed
The new NameLookup field is added to Config and documented as "looks up the archive path for a given mangling name," but nothing invokes it — loadFile discards the config via _ = conf and NewPackage only reads Reused/Importer/Fset. As a public field this can mislead consumers of the cl package into thinking it takes effect. Acceptable as scaffolding if the consumer lands in a follow-up; otherwise consider marking the doc as "reserved / not yet effective," and decide whether nil is permitted (and validated up front) when the callback is eventually wired in.
| } | ||
| */ | ||
|
|
||
| func _TestMockC(t *testing.T) { |
There was a problem hiding this comment.
[P2] Only test of new path (_TestMockC) is disabled
_TestMockC is underscore-prefixed, so go test never runs it. It is the sole test exercising the new cl.Source{TU:} construction, Config.NameLookup, and cltest.TestFromDir. Combined with toType/goNodePos still being panic("todo"), none of the renamed/added code has enforced CI coverage. Fine as scaffolding, but consider a tracking note or enabling a minimal test once the stub panics are implemented so the refactor doesn't silently regress.
| } | ||
| t.Run(name, func(t *testing.T) { | ||
| pkgDir := dir + "/" + name | ||
| if sel != "" && !strings.Contains(pkgDir, sel) { |
There was a problem hiding this comment.
[P3] TestFromDir sel filter runs inside t.Run, emitting empty subtests
The sel != "" && !strings.Contains(pkgDir, sel) check is inside the t.Run(name, ...) closure, so every non-matching directory still spawns an (empty, passing) subtest and returns early rather than being skipped. The doc says "only subdirectories whose path contains sel will be tested," which reads as full exclusion. Behavior is unchanged from the original, but now that this is a shared exported helper, consider filtering before t.Run or using t.Skip to avoid noise-level subtests.
1 participant
No description provided.