Improve test quality: pkg/parser/frontmatter_utils_test.go#23868
Improve test quality: pkg/parser/frontmatter_utils_test.go#23868
Conversation
- Replace t.Errorf/t.Fatalf with assert.*/require.* testify assertions - Add TestIsRepositoryImport with 14 cases - Add TestIsWorkflowSpec with testify-style assertions - Expand TestResolveIncludePath with absolute path security case - Add descriptive messages to all assertions - Fix testifylint require-error lint warning" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4b6fcc6d-5f28-4ddf-82e1-5c03d96acacc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the quality and coverage of pkg/parser/frontmatter_utils_test.go by standardizing assertions with Testify and adding new/expanded test cases for import-path helpers and ResolveIncludePath security behavior.
Changes:
- Replaced
t.Errorf/t.Fatalfusage withassert.*/require.*for clearer, more consistent assertions. - Added
TestIsRepositoryImportwith a table of acceptance/rejection cases. - Expanded
TestResolveIncludePathto include an “absolute path outside base dir” security-rejection case.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestResolveIncludePath(t *testing.T) { | ||
| // Create temporary directory structure | ||
| tempDir, err := os.MkdirTemp("", "test_resolve") | ||
| if err != nil { | ||
| t.Fatalf("Failed to create temp dir: %v", err) | ||
| } | ||
| require.NoError(t, err, "should create temp dir") | ||
| defer os.RemoveAll(tempDir) | ||
|
|
||
| // Create regular test file in temp dir | ||
| regularFile := filepath.Join(tempDir, "regular.md") | ||
| if err := os.WriteFile(regularFile, []byte("test"), 0644); err != nil { | ||
| t.Fatalf("Failed to write regular file: %v", err) | ||
| } | ||
| err = os.WriteFile(regularFile, []byte("test"), 0644) | ||
| require.NoError(t, err, "should write regular file") |
There was a problem hiding this comment.
In this test, baseDir is set to tempDir (which doesn’t include a trailing ".github"). ResolveIncludePath’s security logic is written assuming baseDir is ".github" or a subdirectory (see comment in ResolveIncludePath), so it would be better to create a temp ".github" directory and use that as baseDir to match real usage (and avoid platform-specific path traversal behavior).
| { | ||
| name: "absolute path outside base dir is rejected for security", | ||
| filePath: "/etc/passwd", | ||
| baseDir: tempDir, | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
This case hard-codes "/etc/passwd" as the absolute path. To keep the test platform-agnostic (and avoid relying on Unix-specific paths), consider generating an absolute path that is guaranteed to be outside baseDir using filepath APIs (e.g., an abs path in filepath.Dir(baseDir)).
frontmatter_utils_test.goused rawt.Errorf/t.Fatalfthroughout and lacked coverage forisRepositoryImportand had minimal coverage forResolveIncludePath.Changes
t.Errorf/t.Fatalfwithassert.*/require.*with descriptive messagesTestIsRepositoryImport— new test with 14 cases: validowner/repovariants (with ref, SHA, section, hyphens, underscores) and all rejection paths (3-part workflowspec, local/absolute/shared/prefixed paths, file extensions)TestResolveIncludePath— added absolute path case verifying the security check rejects paths like/etc/passwdthat escape the base directoryTestIsWorkflowSpec— converted to testify assertions with descriptive messagesTestIsNotFoundError— not duplicated here; already covered with 6+ cases inimport_remote_nested_test.go