Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new test helper’s behavior doesn’t match its documented support for relative paths (missing directory creation), and the new test can be made more precise to avoid brittleness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the internal generator input selection so gorm gen ignores Go test files, preventing test-only types (fixtures/fakes) from leaking field helpers into the generated non-test package.
Changes:
- Treats
*_test.gofiles as non-inputs inshouldSkipFile. - Adds a regression test ensuring helpers are generated for non-test structs but not for test-only structs.
- Introduces a small test helper to generate from ad hoc sources into a temp module and read generated output.
File summaries
| File | Description |
|---|---|
internal/gen/utils.go |
Extends shouldSkipFile to exclude *_test.go from generator inputs. |
internal/gen/skip_test_files_test.go |
Adds a test verifying test-only types do not produce helpers. |
internal/gen/generator_support_test.go |
Adds shared test helpers for generating from temporary source modules. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for name, content := range files { | ||
| if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { | ||
| t.Fatalf("write %s: %v", name, err) | ||
| } | ||
| } |
| if strings.Contains(content, "Fixture") { | ||
| t.Fatalf("test-only type Fixture must not get helpers, got:\n%s", content) | ||
| } |
| // shouldSkipFile reports whether a file is not an input for generation: anything that is not Go | ||
| // source, test files (their types exist for tests only and would leak helpers into the output), | ||
| // and files the generator wrote itself. | ||
| func shouldSkipFile(filePath string) bool { | ||
| if !strings.HasSuffix(filePath, ".go") { | ||
| if !strings.HasSuffix(filePath, ".go") || strings.HasSuffix(filePath, "_test.go") { | ||
| return true |
8cf0e56 to
90a9e9f
Compare
|
Addressed the review. The shared test helper now creates parent directories before writing, so a nested key like Also renamed the skip log to "Skipping file", which was still saying "generated file" for a test input. One suggestion not taken: the |
There was a problem hiding this comment.
🟡 Changes recommended
The new test helper can miss generated output in nested directories and one new assertion is overly broad, making the test potentially flaky or misleading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/gen/skip_test_files_test.go:18
- The negative assertion is overly broad: checking for the substring "Fixture" can fail for unrelated reasons if that word appears in some other generated identifier or comment. To make the test intention precise (and consistent with other tests in this package), assert that the specific helper prelude is absent (e.g. "var Fixture = struct").
if strings.Contains(content, "Fixture") {
t.Fatalf("test-only type Fixture must not get helpers, got:\n%s", content)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| return readAllGeneratedGoFiles(t, out) | ||
| } | ||
|
|
||
| // containsField reports whether the generated code declares a helper named name with the given | ||
| // type, tolerating the column alignment gofmt applies to struct fields. | ||
| func containsField(content, name, typ string) bool { | ||
| re := regexp.MustCompile(`(?m)^\s*` + regexp.QuoteMeta(name) + `\s+` + regexp.QuoteMeta(typ) + `\s*$`) | ||
| return re.MatchString(content) | ||
| } |
90a9e9f to
8880f68
Compare
|
Addressed the second round. The shared test helper now reads the whole output tree rather than its top level, so a fixture that spans packages is not silently truncated. That was only reachable once the helper started accepting nested source paths, so thank you for following it through. On the |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the stated problem, and includes a focused regression test that validates the new behavior end-to-end.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Problem
gorm gencollects every.gofile under the input directory, test files included. Types declared for tests only (fixtures, fakes, acommon_test.gonext to the models) therefore get field helpers in the generated package, which then reference structs that exist in no non-test build.Fix
shouldSkipFiletreats_test.gofiles like non-Go files and generated output: not an input for generation.Test
TestProcessSkipsTestFilesruns the generator over a throwaway module holding a model file and a_test.gofile that declares a struct, and asserts the test-only struct gets no helper while the model keeps its own.The first commit adds a small test helper (
generateFromSources,containsField) that writes ad hoc sources into a throwaway module and runs the generator over them. The other pull requests from this fork share that commit; git merges the identical addition without conflict, so they can land in any order.🤖 Generated with Claude Code