fix(tests): corregir fallos de tests pre-existentes en entornos Windows#372
Conversation
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
La idea puede servir, pero falta ordenar el proceso y revisar solapamiento.
Sumá el label type:*, refrescá CI contra main y compará explícitamente este cambio con #490 para que no revisemos dos fixes de Windows que pisan el mismo problema.
|
@Alan-TheGentleman refreshed against current main + addressed the #490 comparison you asked for. RefreshMerge commit: While running the suite against the merged tree I caught a new test introduced by main that the original PR pattern hadn't reached yet:
Extension applied in commit Local validationgo test ./internal/components/sdd/ -run "TestInjectCopiesNestedSDDSkillReferences|TestInjectCopiesAllFiles"
go test ./internal/components/skills/ -run TestSkillPathForAgent
go test ./internal/components/ -run TestGoldenEngram_AntigravityAll 3 previously-failing cases now PASS. The remaining unit-test failure on CI is from Boundary vs #490 (tonyblu331)You asked explicitly for this, so here it is concrete:
Both PRs link
I checked both diffs file-by-file before refreshing — there is no shared symbol, no shared helper, and no shared test name. The two PRs can land in either order without rebase conflicts; whichever lands first the other will merge cleanly. If you'd prefer them landed in a specific order or want me to align helper naming conventions across the two, happy to coordinate with @tonyblu331. Pending maintainer action
|
|
@Basparin — friendly nudge: 10/12 checks green, only 2 failures left. Could you take another pass at the failing jobs and refresh against current |
|
@Basparin — heads-up: this PR's Unit Tests failure is Your branch still expects the old form, so the CI run uses the pre-#660 assertion and fails. Fix: rebase against current Pseudocode: git fetch upstream main
git rebase upstream/main
# resolve any conflicts (likely none for this PR's scope)
git push --force-with-leaseThe other PR you have open (and #371 + #374 + #372) all show the same failure for the same reason — same rebase fixes all three. Thanks for sticking with these. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Thanks for picking this up and preserving @atarico's authorship — the issue-first link (Closes #103, status:approved) and conventional commits are correct, and the approach is sound. .gitattributes eol=lf, filepath.FromSlash, and stubbing npmLookPath are genuine cross-platform fixes, not Windows-only t.Skips — so Linux/macOS coverage stays intact. Good.
Two things block merge:
- Stale / conflicting. The branch is currently conflicting against main (
mergeStateStatus: DIRTY) and needs a rebase.merge-treeshows real conflicts in all three touched test files. - Obsolete hunk. main already fixed
TestSkillPathForAgentindependently usingfilepath.Join(...), so thefilepath.FromSlashhunk inskills/inject_test.gonow conflicts and is redundant — please drop it on rebase.
Also: disablePluginInstall duplicates the existing mockNoPackageManager helper (both stub npmLookPath); please reuse mockNoPackageManager or drop the cases that call both. And the PR description references a golden_test.go "Windows helper" and a regenerated sdd-vscode golden that aren't in the final diff — please update it to match the actual change.
Rebase + drop the obsolete skills hunk and this is good to go.
- Agregar .gitattributes con eol=lf para evitar conversión CRLF en golden files - Regenerar golden files con terminaciones LF consistentes - Usar filepath.FromSlash() en TestSkillPathForAgent para separadores de ruta - Agregar skipIfNoPkgManager() y disablePluginInstall() para tests de OpenCode que requieren npm/bun — ahora hacen skip en lugar de fallar
These two tests exercised the OpenCode inject path without mocking the package manager lookup, so they failed on Windows environments lacking bun/npm with the same unique-names-generator post-install error that atarico addressed for 15+ sibling tests. Applying the same helper keeps the fix for Gentleman-Programming#103 consistent across the whole test file.
Previous regeneration contaminated the golden with persona.Inject output (Rules, Personality, Engram Protocol) that does not belong to sdd.Inject. Restoring to the baseline resolves TestGoldenSDD_VSCode on Linux CI.
…illReferences Main introduced TestInjectCopiesNestedSDDSkillReferences after this PR opened. It exercises the same opencode-plugin install path as its TestInjectCopiesAllFiles* siblings and fails on Windows for the same reason (no bun/npm to install unique-names-generator inside the test temp dir). Apply the same disablePluginInstall(t) helper this PR already uses for the rest of the family. No behavior change in production code.
Basparin
force-pushed
the
fix/103-windows-test-failures
branch
from
5553249 to
d80c72e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesTest Environment Isolation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/sdd/inject_test.go`:
- Around line 3320-3323: Tests call skipIfNoPkgManager() to require a real
package manager but then immediately call mockNoPackageManager(), which
neutralizes that requirement and prevents exercising the install-capable path;
update the two affected tests in inject_test.go (the blocks that call
skipIfNoPkgManager and mockNoPackageManager around lines shown) so they do not
call mockNoPackageManager() before exercising install behavior (either remove
the mockNoPackageManager() call from those tests or move it to after the
install-capable assertions), and apply the same change to the second occurrence
around the other mentioned lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01d7f880-9eba-4d4e-99f5-1bc666f58609
📒 Files selected for processing (3)
.gitattributesinternal/components/golden_test.gointernal/components/sdd/inject_test.go
|
@Alan-TheGentleman review del 29-may atendida completa:
Validación local Windows 11: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/components/sdd/inject_test.go (1)
3307-3324:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
mockNoPackageManagerneutralizesskipIfNoPkgManagerin plugin tests (duplicate concern).These plugin-focused tests first require a real package manager via
skipIfNoPkgManager(t), then immediately mock it away withmockNoPackageManager(t). The result: they only run when bun/npm exist in PATH, but never exercise the install path because the mock forces a no-package-manager code path. This prevents regressions in the install logic from being caught.Suggested fix
Remove
mockNoPackageManager(t)from both tests so they exercise the real install path:func TestInjectOpenCodeSingleWritesPlugin(t *testing.T) { skipIfNoPkgManager(t) home := t.TempDir() - mockNoPackageManager(t) _, err := Inject(home, opencodeAdapter(), "single")func TestInjectOpenCodePluginIdempotent(t *testing.T) { skipIfNoPkgManager(t) home := t.TempDir() - mockNoPackageManager(t) // First run first, err := Inject(home, opencodeAdapter(), "multi")Also applies to: 3429-3454
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/components/sdd/inject_test.go` around lines 3307 - 3324, The test setup calls skipIfNoPkgManager(t) to require a real package manager but then immediately calls mockNoPackageManager(t), which neutralizes that requirement and prevents exercising the install path; remove the mockNoPackageManager(t) call from TestInjectOpenCodeSingleWritesPlugin (and the sibling test around lines 3429-3454) so the tests run only when a package manager is present and actually exercise Inject(..., opencodeAdapter(), "single") install logic instead of the mocked no-package-manager path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/sdd/inject_test.go`:
- Around line 2246-2248: Remove the duplicated consecutive
mockNoPackageManager(t) call in each test; keep a single invocation to install
the mock and register cleanup (e.g., remove the second mockNoPackageManager(t)
after home := t.TempDir() so only the first call remains). Apply the same change
for the other occurrences mentioned (around the ranges referenced) and verify
each test still registers cleanup once and compiles.
---
Duplicate comments:
In `@internal/components/sdd/inject_test.go`:
- Around line 3307-3324: The test setup calls skipIfNoPkgManager(t) to require a
real package manager but then immediately calls mockNoPackageManager(t), which
neutralizes that requirement and prevents exercising the install path; remove
the mockNoPackageManager(t) call from TestInjectOpenCodeSingleWritesPlugin (and
the sibling test around lines 3429-3454) so the tests run only when a package
manager is present and actually exercise Inject(..., opencodeAdapter(),
"single") install logic instead of the mocked no-package-manager path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97037546-026b-4a04-b15d-3cd19261a457
📒 Files selected for processing (1)
internal/components/sdd/inject_test.go
|
Good direction overall. The PR is now clean against current One small cleanup before merge: the final diff leaves duplicate Can you drop the redundant second call in those tests? It does not change behavior, but keeping one setup call per test avoids confusing future readers and makes the Windows-test hardening cleaner. |
…lper consolidation
|
@Alan-TheGentleman done — the 5 duplicate |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Approved. The branch is refreshed against current main, the duplicate mock setup concern is resolved, and the fresh CI/E2E run is green across ubuntu, arch, and fedora.\n\nI also checked the final head locally with ok github.com/gentleman-programming/gentle-ai/internal/components 9.572s
ok github.com/gentleman-programming/gentle-ai/internal/components/sdd 42.816s; both pass. This is now a clean Windows test-hardening slice for #103.
Alan-TheGentleman
merged commit 6893438
into
Gentleman-Programming:main
3 participants
🔗 Linked Issue
Closes #103
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)🙏 Credit
Retoma el trabajo de @atarico en #104 (cerrado por un tema de proceso, no técnico). Su commit original se preserva con autoría intacta (
Author: atarico).📝 Summary
Al correr
go test ./...en Windows hay fallos pre-existentes causados por incompatibilidades del entorno, no por errores de lógica. Tras el rebase sobre main actual (que ya arreglóTestSkillPathForAgentpor su cuenta confilepath.Join), este PR cubre dos categorías:.gitattributes, Git en Windows convierte assets embebidos (go:embed) a CRLF en checkout, rompiendo los tests golden que esperan LF. Solución:.gitattributescon* text eol=lf.bun/npm— los tests de inyección SDD instalan el plugin de background-agents, que falla sin package manager. Solución: reusar el helper existentemockNoPackageManager()en los tests que NO testean el install path, yskipIfNoPkgManager()+ skip por mensaje de error en los que sí lo ejercitan.📂 Changes
.gitattributes* text eol=lfpara forzar LF en todos los sistemasinternal/components/golden_test.got.Skipfen los 2 goldens OpenCode cuando el plugin install no está disponible en el entornointernal/components/sdd/inject_test.goskipIfNoPkgManager()helper +mockNoPackageManager(t)aplicado a 17 tests de inyección que no testean el install pathTotal: 3 files, +59/-0.
Cambios vs versión anterior del PR (review del 29-may):
skills/inject_test.goeliminado — superseded por el fix de main confilepath.Join✅disablePluginInstalleliminado — ahora reusa el helper existentemockNoPackageManager✅🧪 Test Plan
Unit Tests (Windows 11 Pro local, tras rebase sobre main
03457e9)go test ./internal/components/ ./internal/components/sdd/ ./internal/components/skills/gofmt -llimpio en archivos tocadosE2E Tests (Docker required)
✅ Contributor Checklist
status:approved)Co-Authored-BytrailersSummary by CodeRabbit
Chores
Tests