Skip to content

test: enforce package layering and the internal/config data-only rule - #7809

Merged
g-husam merged 6 commits into
mainfrom
test/dependency-rules
Oct 23, 2026
Merged

g-husam merged 6 commits into
mainfrom
test/dependency-rules

Conversation

@g-husam

@g-husam g-husam commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Adds a test that enforces layer to layer dependencies for code hygiene.

This is split out for #7776, which is a skill that generates an interactive walkthrough, a part of which shows each package's dependency graph.

AGENTS.md describes which packages may depend on which, but nothing
failed when a lower layer imported a higher one. dependency_test.go now
lists every package in a top-to-bottom layer table and fails when a
package is missing from the table, when a pattern no longer matches any
package, when an import points upward, when non-test code imports a
test-support package, or when anything other than internal/yaml or
internal/git imports go.yaml.in or go-git. A second test parses
internal/config and fails on any function or method declaration.

Each failure names the package, the layer and the fix. The rules were
checked against the current import graph (which already obeys them) and
by injecting an upward import, an unlisted package, a stale pattern, a
production import of internal/testhelper, a direct yaml import and a
function in internal/config; every case fails with the intended message.
Root-package tests are not covered by the coverage job, which excludes
the root package, so each one needs its own job in linter.yaml like the
existing lint checks. Adds a dependencies job for TestDependencyRules
and TestConfigIsPureData.

In dependency_test.go, rename patterns to pathPatterns and print the
expected order when a layer's patterns are unsorted.
Signed-off-by: g-husam <husameldawi@google.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces dependency_test.go to enforce package layering rules and ensure that internal/config remains a pure data package. A high-severity issue was identified in the third-party dependency gates where the YAML package import path was incorrectly specified as "go.yaml.in/yaml" instead of "gopkg.in/yaml", which would cause the dependency check to be silently bypassed.

Comment thread dependency_test.go Resolved
TestDependencyRules checked six things at once, so a failure named the
file but not the rule. Each rule is now its own TestLayering* function
with Given/When/Then comments: the table is sorted, every package has
exactly one layer, every pattern matches a package, imports point
downward, production code does not import test support, third-party
libraries come in only through their wrapper, and internal/config
declares no functions. go list runs once and is shared.

The CI job runs go test -run '^TestLayering' so new rules with that
prefix are picked up without editing the workflow.
Comment thread dependency_test.go Resolved
Comment thread dependency_test.go Resolved
Move the When comment in TestLayeringEveryPackageHasExactlyOneLayer below
the testSupport skip check so it precedes the layer-matching loop, and
drop the redundant testSupport suffix from the When comment in
TestLayeringProductionCodeDoesNotImportTestSupport.
Add blank lines between multi-line Given, When, and Then sections in
dependency_test.go, and clarify the Then comment in
TestLayeringEveryPackageHasExactlyOneLayer to refer to layers rather
than entries.
@g-husam
g-husam added this pull request to stack #7810 October 23, 2026 16:18
@g-husam
g-husam marked this pull request as ready for review October 23, 2026 16:24
@g-husam
g-husam requested a review from a team as a code owner October 23, 2026 16:24
@g-husam
g-husam requested a review from noahdietz October 23, 2026 16:24

@noahdietz noahdietz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I love the 3p check, and otherwise I think this is helping enforce a clean intra project dependency graph and organization. Nice

@g-husam
g-husam merged commit 756dde1 into main Oct 23, 2026
93 of 94 checks passed
@g-husam
g-husam deleted the test/dependency-rules branch October 23, 2026 16:53
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