Skip to content

test: add unit tests for remy provider [IDE-2052]#1330

Draft
bastiandoetsch wants to merge 2 commits into
feat/IDE-2052-pr4from
feat/IDE-2052-pr5
Draft

test: add unit tests for remy provider [IDE-2052]#1330
bastiandoetsch wants to merge 2 commits into
feat/IDE-2052-pr4from
feat/IDE-2052-pr5

Conversation

@bastiandoetsch

@bastiandoetsch bastiandoetsch commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

  • 20+ unit tests in domain/snyk/remediation/remy_test.go covering:
    • Guards (empty FindingId/ContentRoot/FilePath, non-absolute path, non-git-repo)
    • Worktree isolation (runner receives worktree path, not real workspace)
    • Multi-file partitioning and cache serving (runner called once on cache hit)
    • Cache eviction via InvalidateFile and stat-based mtime invalidation
    • Subdir workspace: ContentRoot is monorepo subfolder, Changes keyed by gitRoot/relPath
    • parseDiffHunks regression: deletion of -- lines not silently dropped
    • parseDiffHunks edge cases: malformed header, no-newline marker, consecutive insertions

PR Stack — Merge Order

flowchart LR
    main(["main"])
    PR1["#1326 PR-1\nfindingId + kind"]
    PR2["#1327 PR-2\nCA provider"]
    PR3["#1328 PR-3\nremy initial"]
    PR4["#1329 PR-4\nworktree impl"]
    PR5["#? PR-5 ← YOU ARE HERE\nunit tests"]
    PR6["#? PR-6\ninteg+smoke tests"]
    main --> PR1 --> PR2 --> PR3 --> PR4 --> PR5 --> PR6
    style PR5 fill:#ffd700,color:#000
Loading

Depends on: #1329

Test plan

  • go test -race ./domain/snyk/remediation/... passes — 83%+ coverage

PR Type

Tests


Description

  • Add comprehensive unit tests for remediation.RemiProvider.

  • Cover input validation, worktree isolation, caching, and diff parsing edge cases.

  • Refactor shared git helper functions into a common test file.


File Walkthrough

Relevant files
Tests
remy_test.go
Add comprehensive unit tests for remy provider                     

domain/snyk/remediation/remy_test.go

  • Introduces a new file containing a comprehensive suite of unit tests
    for the remediation.RemiProvider.
  • Tests cover a wide range of scenarios including input validation,
    worktree isolation, caching mechanisms, sub-directory workspaces, and
    various diff parsing edge cases.
  • Includes shared helper functions for git repository setup and fake
    runners.
+666/-0 
Refactoring
remy_provider_test.go
Remove duplicated git helpers                                                       

domain/snyk/remediation/remy_provider_test.go

  • Helper functions initGitRepo and commitFile were removed from this
    file.
  • These helpers are now defined and shared in the new remy_test.go file.
+1/-35   

@snyk-io

snyk-io Bot commented Jun 8, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@bastiandoetsch

Copy link
Copy Markdown
Contributor Author

/describe

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (2c0fbaf)

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Compilation Error 🔴 [critical]

The test file attempts to call remediation.ExportedWorkspaceEditFromContent (and other Exported* variants) which are not defined in the codebase. The actual implementation in remy.go defines workspaceEditFromContent as an unexported function. Because remy_test.go is in the remediation_test package, it cannot access unexported symbols, and without an export_test.go file (not included in this PR), the package will fail to compile.

edit, err := remediation.ExportedWorkspaceEditFromContent(absPath, original, diff)
Logic Bug 🟠 [major]

TestRemyProvider_MultiFileChange_PartitionsAndCaches validates a logic error where the remediation for one finding is incorrectly applied to another. The test asserts that a request for finding-b should be served from the cache populated by finding-a (line 319). In a real scenario, this would apply the LLM fix meant for one vulnerability to a different issue if they happen to touch the same file. The cache must be isolated by FindingId or invalidated when the requested finding changes.

	FindingId:   "finding-b",
	ContentRoot: types.FilePath(repoRoot),
	FilePath:    types.FilePath(absB),
})
require.NoError(t, err)
require.NotNil(t, editB, "second call must return cached changes for b.go")
assert.Contains(t, editB.Changes, absB, "second edit must be keyed by b.go workspace path")
assert.NotContains(t, editB.Changes, absA, "second edit must not include a.go")
assert.Equal(t, 1, runnerCalls, "runner must NOT be called again on cache hit")
📚 Repository Context Analyzed

This review considered 12 relevant code sections from 11 files (average relevance: 1.00)

@bastiandoetsch

Copy link
Copy Markdown
Contributor Author

/describe

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (72120e8)

@bastiandoetsch

Copy link
Copy Markdown
Contributor Author

Addressing bot finding — Compilation Error (ExportedWorkspaceEditFromContent):

The export_for_test.go file exists in domain/snyk/remediation/ (introduced in the test(remediation): add unit tests for remy provider commit). It exports workspaceEditFromContent as ExportedWorkspaceEditFromContent for black-box tests in the remediation_test package.

The bot finding was raised against an earlier commit. The current PR head (72120e8d) builds and tests cleanly:

go test ./domain/snyk/remediation/... ✅

No fix needed — false positive against stale commit.

@basti-snyk

Copy link
Copy Markdown
Contributor

/describe

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (0e94ef1)

Comprehensive unit test coverage for domain/snyk/remediation:
- Guards: empty FindingId, ContentRoot, FilePath, non-absolute path,
  non-git-repo ContentRoot
- Worktree isolation: runner receives isolated worktree path, not real workspace
- Runner error propagation
- No-changes → nil edit
- Single-file WorkspaceEdit keyed by real workspace path (not worktree path)
- Multi-file partitioning and cache serving (runner called exactly once on
  cache hit)
- Cache eviction via InvalidateFile (runner re-invoked after file evicted)
- Stat-based cache invalidation (file deleted → cacheValid fails → re-run)
- Subdir workspace: ContentRoot is monorepo subfolder, Changes keyed by
  gitRoot/relPath
- parseDiffHunks: malformed header, no-newline marker, consecutive insertion
  merge, negative startLine, startLine exceeds file length
- Regression: deletion of "--" lines (SQL comments) not dropped as header
- workspaceEditFromContent: empty content/diff errors
- NewRemyProvider with nil runner sets default gafRunner
- FileChangeNotifier interface implemented by remyProvider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

depends-on-1329 Depends on PR #1329

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants