docs(#6197): document Fn suffix convention for injectable function variables - #6200
docs(#6197): document Fn suffix convention for injectable function variables#6200fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
…riables Add an "Injectable function variables (test seams)" section to docs/contributing/go-code.md documenting the naming and usage convention for package-level function variables used as test seams. The convention (XxxFn suffix, default to real impl, doc comment pattern, t.Cleanup restoration) has 9+ instances with zero violations but was previously undocumented, causing agents to infer the pattern from code rather than referencing written guidance. Note: pre-commit could not run in sandbox (network restriction). Manual verification of pre-commit is required. Closes #6197
|
🤖 Finished Review · ✅ Success · Started 6:07 PM UTC · Completed 6:19 PM UTC Commit: |
Site previewPreview: https://4e50ca7e-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Labels: PR modifies documentation under docs/contributing/ about Go code conventions |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| Package-level variables that hold function values for test overriding must: | ||
|
|
||
| - Use an `XxxFn` suffix (e.g., `BuildWASMFn`, `RetrySleepFn`, `WranglerWhoamiFn`) | ||
| - Default to the real implementation |
There was a problem hiding this comment.
[medium] accuracy
The doc comment requirement says comments must 'start with "Override in tests to..."' but every cited example in the codebase follows a different pattern: they start with the variable name per Go convention, then include 'Override in tests to...' as a later sentence. The phrasing 'starting with' is inaccurate relative to the codebase's own examples.
Suggested fix: Change 'Include a doc comment starting with "Override in tests to..."' to 'Include a doc comment following Go convention (starting with the variable name) that contains an "Override in tests to..." sentence describing the override behavior.'
| ## Injectable function variables (test seams) | ||
|
|
||
| Package-level variables that hold function values for test overriding must: | ||
|
|
There was a problem hiding this comment.
[low] stale-reference
The documentation states that injectable function variables must use an XxxFn suffix (capitalized/exported), but internal/sandbox/gateway_endpoint.go has resolveOverrideFn (lowercase/unexported) that also serves as a test seam following the same pattern. The documented convention does not account for unexported test seams.
Suggested fix: Add a note that this convention applies to exported/newly-introduced test seams, or clarify that lowercase xxxFn variables are a legacy pattern.
fullsend-ai-review
Bot
added
requires-manual-review
Summary
Documents the
XxxFnsuffix naming convention for injectable function variables (test seams) indocs/contributing/go-code.md. This convention has 9+ consistent instances acrossinternal/sandbox/andinternal/dispatch/cf/but was previously undocumented, causing agents to infer it from code patterns rather than referencing written guidance.Changes
docs/contributing/go-code.mdafter the "Concurrency testing" sectionFnsuffix, default to real implementation, "Override in tests to..." doc comment pattern, andt.CleanuprestorationRetrySleepFn,BuildWASMFn,CopyWASMExecFn)Testing
Checklist
!for breaking changes)Closes #6197
Post-script verification
agent/6197-doc-fn-suffix-convention)ed0deb850458a4eb2107b51298a569c7503295db..HEAD)