test: improve render coverage from 78.5% to 86.9%#67
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5e4689f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var buf bytes.Buffer | ||
| a := NewCloneAnimation(&buf, "repo") | ||
|
|
||
| a.Demo() |
There was a problem hiding this comment.
TestCloneAnimationDemo invokes Demo(), which sleeps 80ms for each of 51 frames plus a final 500ms pause, adding about 4.6 seconds to every run of this package even though the assertion only checks final output content. This materially slows local/CI feedback loops and is avoidable by testing Render/buildFrame directly or injecting a mock sleeper/clock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR increases render-related test coverage by adding new deterministic test cases for the skyline animation/model and clone animation demo, and raises the CI coverage floor accordingly.
Changes:
- Added skyline tests covering phase-2 transitions, shooting star rendering, project-name fallback, and the stdout-only animation path.
- Added a clone animation demo test to cover completion/newline behavior.
- Raised the enforced CI total coverage floor from 45.0% to 50.0%.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
render/skyline_test.go |
Adds tests for skyline model/view behavior and additional Skyline rendering paths. |
render/clone_animation_test.go |
Adds a demo test to cover end-of-demo output behavior. |
.github/workflows/ci.yml |
Raises the enforced total coverage minimum to 50.0%. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var buf bytes.Buffer | ||
| a := NewCloneAnimation(&buf, "repo") | ||
|
|
||
| a.Demo() | ||
|
|
||
| out := buf.String() | ||
| if !strings.Contains(out, "100%") { | ||
| t.Fatalf("expected demo output to reach 100%%, got %q", out) | ||
| } | ||
| if !strings.HasSuffix(out, "\n") { | ||
| t.Fatalf("expected demo output to end with newline, got %q", out) | ||
| } |
There was a problem hiding this comment.
This test calls CloneAnimation.Demo(), which currently sleeps ~4.6s (80ms * 51 steps + 500ms). That will noticeably slow the test suite/CI and can make failures take longer to surface. Consider refactoring Demo to allow a zero-delay sleep function (or duration) to be injected for tests, or avoid calling Demo directly and instead assert the intended end-state without real time.Sleep calls.
| var buf bytes.Buffer | |
| a := NewCloneAnimation(&buf, "repo") | |
| a.Demo() | |
| out := buf.String() | |
| if !strings.Contains(out, "100%") { | |
| t.Fatalf("expected demo output to reach 100%%, got %q", out) | |
| } | |
| if !strings.HasSuffix(out, "\n") { | |
| t.Fatalf("expected demo output to end with newline, got %q", out) | |
| } | |
| t.Skip("Demo() performs real time.Sleep calls; skipping in unit tests to keep the suite fast") |
| origStdout := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| os.Stdout = w | ||
| t.Cleanup(func() { | ||
| os.Stdout = origStdout | ||
| }) | ||
|
|
||
| done := make(chan string, 1) | ||
| go func() { | ||
| data, _ := io.ReadAll(r) | ||
| done <- string(data) | ||
| }() | ||
|
|
||
| Skyline(w, project, true) | ||
|
|
||
| if err := w.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
This test uses os.Pipe and replaces the global os.Stdout, but it never closes the read end (r) and it only closes w on the happy path. This can leak file descriptors and can also leave the reader goroutine blocked if the test exits early. Prefer the repo’s temp-file stdout capture pattern (e.g., cmd/hooks_test.go captureOutput) and ensure both pipe ends are closed via t.Cleanup if you keep the pipe approach.
| func TestSkylineAnimatePathCallsRenderAnimatedForStdout(t *testing.T) { | ||
| project := scanner.Project{ | ||
| Root: t.TempDir(), | ||
| Name: "Demo", | ||
| Files: []scanner.FileInfo{ | ||
| {Path: "main.go", Ext: ".go", Size: 100}, | ||
| }, | ||
| } | ||
|
|
||
| origStdout := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| os.Stdout = w | ||
| t.Cleanup(func() { | ||
| os.Stdout = origStdout | ||
| }) | ||
|
|
||
| done := make(chan string, 1) | ||
| go func() { | ||
| data, _ := io.ReadAll(r) | ||
| done <- string(data) | ||
| }() | ||
|
|
||
| Skyline(w, project, true) | ||
|
|
||
| if err := w.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| out := <-done | ||
| if !strings.Contains(out, "Demo") { | ||
| t.Fatalf("expected skyline output to include project name, got:\n%s", out) | ||
| } | ||
| } |
There was a problem hiding this comment.
As written, this test is likely to take multiple seconds because Skyline(..., animate=true) runs the full Bubble Tea animation loop (tick interval 60ms, ~40+ frames). It also doesn’t strongly assert that the animated path was chosen, since the static path would still include the project name. Consider adding a test seam (e.g., a package-level var for renderAnimated) so the test can assert invocation and complete quickly without running Bubble Tea in real time.
| run: | | ||
| total=$(go tool cover -func=coverage.out | awk '/^total:/ {gsub("%","",$3); print $3}') | ||
| # Current enforced coverage floor. Codex PRs raise this incrementally toward 90%. | ||
| min=45.0 | ||
| min=50.0 | ||
| awk -v t="$total" -v m="$min" 'BEGIN { | ||
| if (t+0 < m+0) { |
There was a problem hiding this comment.
The coverage floor was raised to 50.0, but the badge config still uses minColorRange: 45 (below in this file). If minColorRange is intended to reflect the enforced floor, consider updating it to 50 to keep CI messaging consistent.
Summary
.github/workflows/ci.ymlfrom45.0to50.0(total coverage now exceeds floor + 5).Coverage
Validation
go test -race -coverprofile=coverage.out ./...go vet ./...gofmt -l .