Improve test coverage and structure for pkg/parser/import_cache_test.go#18369
Improve test coverage and structure for pkg/parser/import_cache_test.go#18369
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves the quality and coverage of the import cache unit tests by restructuring existing tests into clearer subtests and adding additional edge-case coverage around path sanitization/validation and cache overwrite behavior.
Changes:
- Split
TestImportCacheintot.Run()subtests for clearer failure localization. - Expanded table-driven tests for
sanitizePathandvalidatePathComponentswith additional edge cases. - Added new tests for
Setidempotent overwrite behavior and forGetbehavior with suspicious (non-validated) inputs.
Comments suppressed due to low confidence (1)
pkg/parser/import_cache_test.go:51
- The error returned by Set is ignored here (
cachedPath, _ := ...). Userequire.NoErrorbefore creating the second cache instance so failures point to the real cause instead of a later "found" assertion.
t.Run("New cache instance finds existing entry", func(t *testing.T) {
cachedPath, _ := cache.Set(owner, repo, path, sha, testContent)
cache2 := NewImportCache(tempDir)
retrievedPath2, found := cache2.Get(owner, repo, path, sha)
assert.True(t, found, "cache entry should be found from new cache instance")
assert.Equal(t, cachedPath, retrievedPath2, "path from new instance should match original")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Run("Get returns cached path after Set", func(t *testing.T) { | ||
| cachedPath, _ := cache.Set(owner, repo, path, sha, testContent) | ||
| retrievedPath, found := cache.Get(owner, repo, path, sha) | ||
| assert.True(t, found, "cache entry should be found after Set") | ||
| assert.Equal(t, cachedPath, retrievedPath, "retrieved path should match path returned by Set") |
There was a problem hiding this comment.
In this subtest the error returned by Set is discarded (cachedPath, _ := ...). If Set starts failing, the test will fail later with a less-informative assertion. Prefer require.NoError (or at least check the error) before using cachedPath to keep failures actionable.
This issue also appears on line 46 of the same file.
| // If validation is added in the future, this test should be updated. | ||
| tempDir := t.TempDir() | ||
| cache := NewImportCache(tempDir) | ||
|
|
||
| // Should return not-found (not panic or error), even with suspicious inputs. |
There was a problem hiding this comment.
This test name/comment claims to document that Get does not validate components, but the only assertion is found == false for a key that was never written. Even if Get did validate in the future, it would likely still return found=false, so the test wouldn’t catch the behavior change. Consider renaming the test to reflect what is actually asserted (e.g., "Get returns not-found for suspicious inputs without panicking"), or strengthen it to specifically exercise the validation asymmetry.
| // If validation is added in the future, this test should be updated. | |
| tempDir := t.TempDir() | |
| cache := NewImportCache(tempDir) | |
| // Should return not-found (not panic or error), even with suspicious inputs. | |
| // Specifically, Set should reject suspicious inputs, while Get should | |
| // simply return not-found (and not panic) for the same inputs. | |
| tempDir := t.TempDir() | |
| cache := NewImportCache(tempDir) | |
| // First, verify that Set rejects suspicious inputs via validation. | |
| _, err := cache.Set("../etc", "repo", "test.md", "sha", []byte("content")) | |
| require.Error(t, err, "Set should reject suspicious owner/path input") | |
| // Then, verify that Get with the same suspicious inputs just returns not-found, | |
| // rather than validating and erroring or panicking. |
The import cache test file had a monolithic test function, missing edge cases in table-driven tests, and no coverage for idempotent overwrites or the
Get/Setvalidation asymmetry.Changes
TestImportCache— split into fourt.Run()subtests: file creation, Get-after-Set roundtrip, content verification, cross-instance hitTestSanitizePath— added edge cases: empty string (→ "."), trailing slash, single dot component, leading slash (→ "_absolute_path")TestValidatePathComponents— added missing error cases: emptyrepo, emptypath, and..embedded within ashavalue ("abc..def")TestImportCacheSet_Validation— added one positive (non-error) case to confirm valid inputs pass throughTestImportCacheSetIdempotent— new test verifying that a secondSetwith the same key succeeds, returns the same path, and overwrites file contentTestImportCacheGetDoesNotValidateComponents— new test documenting thatGetskips validation (unlikeSet), and that path-traversal inputs returnfound=falsewithout panickingWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/graphql/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw(http block)https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha iK_VzgSGT -tests 0163866/b387/_pkg_.a(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v3/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha sole.test(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha vaScript2318284702/001/test-fron-s /tmp/go-build2920677756/b296/vet-w Name,createdAt,startedAt,updated-buildmode=exe ./../.prettieriggit(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha vaScript2318284702/001/test-frontmatter-with-arrays.md -buildtags /opt/hostedtoolcache/node/24.13.0/x64/bin/npx ./../.prettieriggit -ifaceassert -nilfunc npx pret�� --write **/*.cjs 0163866/b373/vet.cfg **/*.json --ignore-path ../../../.pretti--show-toplevel sh(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha image:v1.0.0 x_amd64/vet /usr/bin/git(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v5/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha '**/*.ts' '**/*.json' --ignore-premote.origin.url 0677756/b032/vet.cfg 0/x64/bin/node(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha -bool -buildtags /usr/bin/git -errorsas -ifaceassert -nilfunc git -C runs/20260225-160211-6755/test-2406991587 config /usr/bin/git remote.origin.urgit(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha user.email test@example.com /usr/bin/git(http block)https://api.github.com/repos/actions/github-script/git/ref/tags/v8/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha -c=4 -nolocalimports -importcfg /tmp/go-build3880163866/b395/importcfg -pack /home/REDACTED/work/gh-aw/gh-aw/pkg/fileutil/fileutil.go /home/REDACTED/work/gh-aw/gh-aw/pkg/fileutil/tar.go(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha ath ../../../.pr**/*.json(http block)https://api.github.com/repos/actions/setup-go/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha -bool -buildtags /home/REDACTED/.dotnet/tools/sh ./../.prettieriggit -ifaceassert -nilfunc sh -c 0211-6755/test-2406991587 -tests 0163866/b362/vet.cfg l(http block)https://api.github.com/repos/actions/setup-node/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha vaScript2318284702/001/test-fron-test.timeout=10m0s /tmp/go-build2920677756/b233/vet-test.run=^Test /home/REDACTED/.config/composer/ve-test.short=true ./../.prettieriggit(http block)https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts/usr/bin/gh gh run download 1 --dir test-logs/run-1 .cfg x_amd64/compile(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts/usr/bin/gh gh run download 12345 --dir test-logs/run-12345 .cfg 64/bin/bash(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts/usr/bin/gh gh run download 12346 --dir test-logs/run-12346 .cfg 0/x64/bin/bash(http block)https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts/usr/bin/gh gh run download 2 --dir test-logs/run-2 .cfg x_amd64/compile(http block)https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts/usr/bin/gh gh run download 3 --dir test-logs/run-3 .cfg ache/go/1.25.0/x64/pkg/tool/linu-lang=go1.25(http block)https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts/usr/bin/gh gh run download 4 --dir test-logs/run-4 .cfg x_amd64/link(http block)https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts/usr/bin/gh gh run download 5 --dir test-logs/run-5 .cfg ache/go/1.25.0/x64/bin/go(http block)https://api.github.com/repos/github/gh-aw/actions/workflows/usr/bin/gh gh workflow list --json name,state,path(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq .object.sha celain --ignore-.github/workflows/test.md .cfg tions/setup/node-nolocalimports(http block)https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999/usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq .object.sha celain --ignore-@{u}(http block)https://api.github.com/repos/nonexistent/repo/actions/runs/12345/usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion(http block)https://api.github.com/repos/owner/repo/actions/workflows/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo x_amd64/vet(http block)/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo /sh(http block)https://api.github.com/repos/owner/repo/contents/file.md/tmp/go-build3880163866/b381/cli.test /tmp/go-build3880163866/b381/cli.test -test.testlogfile=/tmp/go-build3880163866/b381/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true(http block)https://api.github.com/repos/test-owner/test-repo/actions/secrets/usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name(http block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>[testify-expert] Improve Test Quality: pkg/parser/import_cache_test.go</issue_title>
<issue_description>The test file
pkg/parser/import_cache_test.gohas been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability.Current State
pkg/parser/import_cache_test.go(289 lines)pkg/parser/import_cache.go(175 lines)//go:build !integrationassertandrequireimportedStrengths ✅
require/assertsplit — Critical setup steps (file creation, directory creation) userequire.NoError, while validation assertions useassert. This is exactly the right pattern.TestSanitizePath,TestValidatePathComponents, andTestImportCacheSet_Validationall use table-driven tests witht.Run(), making them easy to extend.Areas for Improvement 🎯
1.
TestImportCacheshould use subtests viat.Run()Current Issue:
TestImportCachetests 4 distinct behaviors in sequence (Set/Get roundtrip, file creation, content verification, cross-instance cache hit) without usingt.Run(). When this test fails, it's hard to know which specific behavior is broken.Recommended Change:
2. Missing test cases in
TestSanitizePathCurrent Issue: The table only tests 4 cases. Several important edge cases are missing.
Missing cases:
{ name: "empty string", input: "", expected: ".", // filepath.Clean("") == "." }, { name: "trailing slash", input: "a/b/", expected: "a_b", }, { name: "single dot component", input: "a/./b", expected: "a_b", }, { name: "leading slash becomes root-like", input: "/absolute/path", expected: "_absolute_path", },3. Missing test cases in
TestValidatePathComponentsCurrent Issue: The table covers
ownerandshavalidation but misses:repopath..embedded insha(e.g."abc..def")Missing cases:
{ name: "empty repo", owner: "testowner", repo: "", path: "test.md", sha: "abc123", shouldErr: true, errMsg: "empty component", }, { name: "empty path", owner: "testowner", repo: "testrepo", path: "", sha: "abc123", shouldErr: true... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes github/gh-aw#18365 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.