Skip to content

testing: add tests - #1

Draft
mohammed90 wants to merge 4 commits into
masterfrom
add-tests
Draft

testing: add tests#1
mohammed90 wants to merge 4 commits into
masterfrom
add-tests

Conversation

@mohammed90

Copy link
Copy Markdown
Member

Disclosure: LLM generated (copilot/Claude Fable 5)

ℹ️ Draft until I review every single line

Adds a correctness-asserting test suite (7 new test files, 7 new testdata fixture packages) covering edge cases across all source files, plus a CI workflow. Every bug found is listed below with the test that covers it.

Unlike the existing baseline tests (which pass on current behavior), these tests assert correct behavior and intentionally fail red until the underlying bug is fixed. Toolchain-dependent tests are skipped under go test -short; crash-prone cases (stack overflows, fatal map races) run isolated in child processes so they cannot take down the rest of the suite.

New tests

File Covers
utils_edge_test.go SplitLastDot, ConfigPathParts, JSON/caddy tag parsing, FQTN helpers on non-named types
storage_edge_test.go dereference/deepDereference: versioned refs, not-found, namespace transfer, doc merging, cycles; includes a reusable thread-safe in-memory mock Storage
traverse_edge_test.go TraverseType: start validation, struct fields, transparent map/array containers, namespaced module lookup, inline keys, unknown module IDs
concurrency_edge_test.go Shared-Driver representation building, module type loading, and workspace package loading under -race
sourcecode_edge_test.go Module ident discovery: pointer/value registration, constant and concatenated module IDs, cross-package registration, strict-validation contract
synthesis_edge_test.go buildRepresentation: ptr-to-ptr, json:"-", unexported fields, non-string map keys, interface{}, RawMessage→Module, ModuleMap, embedded promotion, inline structs, recursive types, func/chan fields
workspace_edge_test.go Temp-dir lifecycle, cache partial-miss, versioned package keys, module-hierarchy matching

Bugs found (B1–B12)

Confirmed, with red tests:

  • B1 (critical): Driver.mu is declared but never used; discoveredTypes is accessed unlocked — data race confirmed by -race (TestConcurrentDiscoveredTypesAccess)
  • B2 (critical): TraverseType indexes vals[0] without a length check — panics on unknown module IDs (TestTraverseTypeUnknownModuleID)
  • B5: func/chan fields hard-fail the entire containing type; no fallback (TestBuildRepresentationUnsupportedFieldTypes)
  • B6: unchecked AST type assertions — caddy.RegisterModule(otherpkg.Type{}) panics with an interface-conversion error (TestFindModuleIdentsUnusualAST)
  • B7: module IDs from constants or string concatenation are silently skipped (TestModuleIDFromConstant, TestModuleIDFromConcatenation)
  • B9: deepDereference has no cycle detection — stack overflow (TestDeepDereferenceCycle)
  • B10: jsonNameFromTag comma handling diverges from encoding/json (TestJSONNameFromTagEdgeCases)
  • B11 (new find): dereference mutates Storage-owned values in place — doc duplication, namespace clobbering across contexts, and a second data race under concurrent LoadTypesByModuleID (TestDereferenceDoesNotMutateStoredType, TestConcurrentModuleTypeLoading)
  • B12 (new find): buildRepresentation has no recursion guard — a self-referential struct (type Node struct { Next *Node }) stack-overflows (TestBuildRepresentationRecursiveType)

Documented as possibly intended (green tests lock in the current contract):

  • B8: one incomplete module (registration without implementation, or vice versa) fails the entire package load (TestUnregisteredModuleFailsPackage, TestRegistrationWithoutLocalImplementation)

Investigated and disproven:

  • B3: the claimed temp-dir leak on go mod init failure does not exist
  • B4: the claimed workspace cache race does not exist (access is properly locked); only unbounded growth remains
  • The previously reported nil-deref in dereference does not exist

Other changes

  • CI workflow (.github/workflows/ci.yml): follows the caddyserver/caddy CI pattern (harden-runner, SHA-pinned actions, linux matrix, Go 1.27, go vet + go test -short -race). Note: CI stays red until the bugs above are fixed — intentional per the red-test policy

Test results

  • go test -short ./ — passes except 5 intentional reds
  • go test ./ — 10 intentional reds
  • go test -race ./ — 12 intentional reds (adds the two concurrency tests)
  • go vet clean

Notes for reviewers

  • No source files were modified — this PR is tests, fixtures, CI, and doc corrections only
  • Every red test maps to a bug above; fixing a bug should flip its test(s) green with no test changes needed (B8 documents current strict validation and may be reclassified as intended behavior)

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
@mohammed90

Copy link
Copy Markdown
Member Author

The commit 3034714 fixes all 9 bugs that the edge-case suite flagged red. The full suite,
go test -race, go vet, and gofmt are now clean — including CI.

Source changes

Thread safety

  • Driver.discoveredTypes is now accessed only through mutex-guarded helpers
    (discoveredType / setDiscoveredType / deleteDiscoveredType); the
    long-standing "CONFIRMED race" TODO is gone (driver.go, synthesis.go)
  • dereference no longer mutates Storage-owned values: it works on a deep copy
    via the new Value.clone(), which also removes the doc-duplication and
    namespace-clobbering bugs and makes concurrent LoadTypesByModuleID safe
    (storage.go); TraverseType's module lookup clones for the same reason

Panics and crashes

  • TraverseType returns a clear error for unknown module IDs instead of
    panicking on vals[0] (driver.go)
  • All AST type assertions are checked: cross-package registrations like
    caddy.RegisterModule(otherpkg.Type{}) are skipped with a warning (they're
    documented from their home package), non-ident receivers error cleanly
    (sourcecode.go)
  • deepDereference tracks the resolution path and returns a
    circular type reference error on SameAs cycles instead of overflowing the
    stack (storage.go)
  • buildRepresentation pre-registers a type before building its fields, so
    self-referential structs (type Node struct { Next *Node }) resolve to a
    reference instead of recursing forever; the placeholder is removed if the
    build fails (synthesis.go)

Behavior corrections

  • Module IDs are now evaluated as constant expressions via go/types, so
    package constants and constant concatenations work, with the old
    string-literal path kept as a fallback for packages whose type info is
    unavailable; only truly runtime-computed IDs are still skipped (sourcecode.go)
  • chan/func/generic struct fields no longer fail the whole containing type;
    they fall back to an empty representation, same as interfaces (synthesis.go)
  • jsonNameFromTag now matches encoding/json semantics: only a tag of exactly
    "-" excludes a field, and the name is cut at a comma in any position
    (json:",omitempty""", json:"-,""-") (utils.go)

Test changes

  • No edge-case test assertions were changed — all 11 previously-red tests pass
    as written, which was the point of the red-test policy
  • Two baseline cases that had locked in old behavior were updated:
    json:"-,omitempty" now expects stdlib semantics, and the tag-parsing
    expectations align with the fix
  • Worth noting: the baseline ValidModulePattern test caught a real regression
    during this work — the first version of the constant-ID change dropped the
    literal fallback, which broke packages that load with type errors. The
    fallback is required and now covered.

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.

1 participant