Test Coverage PR-1: Tests added in ./licensecheck and ./repodata subdir by mohanmanikanta2299 · Pull Request #231 · hashicorp/copywrite

mohanmanikanta2299 · 2026-06-03T09:40:45Z

🛠️ Description

Summary

This PR is the 1st part of the test coverage PRs intended to increase the test coverage of the entire repo from 55% to 80+%, fulfilling our coverage requirements.

Changes Made

Implemented comprehensive table-driven tests for the ./licensecheck and ./repodata packages.
Added validation for CLI command flags, ensuring default values and flag bindings are properly configured.
Added strict assertions (assert.Equal, assert.Contains) and explicit error checking to validate function outputs and edge cases.
Used require for critical initializations (like loggers and flag lookups) to prevent nil-pointer panics during test execution.

Testing

Ran go test -v ./... (All tests passing)
Ran go test -race ./... (No race conditions detected)
Verified coverage metrics locally.

🔗 External Links

https://hashicorp.atlassian.net/browse/CCEN-512

👍 Definition of Done

Tests added : ✅

🤔 Can be merged upon approval?

✅

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

…ectories

CreatorHead · 2026-06-03T10:20:45Z

+		// Header should come before original content
+		headerIdx := lo.IndexOf([]byte(string(content)), []byte(header)[0])
+		contentIdx := lo.IndexOf([]byte(string(content)), []byte(originalContent)[0])
+		assert.Less(t, headerIdx, contentIdx)


Hey @mohanmanikanta2299, I changed header to just "C" in the "add header to file with existing content" sub-test and the test still passed. Please take a look and review the PR properly before sending it for review.

aatish@aatish-CLPJCQ9LWW copywrite % go test ./licensecheck/ -run "TestAddHeader/add_header_to_file_with_existing_content" -v -count=1 === RUN TestAddHeader === RUN TestAddHeader/add_header_to_file_with_existing_content --- PASS: TestAddHeader (0.00s) --- PASS: TestAddHeader/add_header_to_file_with_existing_content (0.00s) PASS ok github.com/hashicorp/copywrite/licensecheck 0.971s

@mohanmanikanta2299 , please enable co-pilot review as well, it will help saving some time in review from other Devs.

Addressed the concern.
Thanks

This is still not fixed — assert.Contains with "C" as the header still passes since "C" exists in the original content.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

CreatorHead · 2026-06-12T07:55:43Z

+		}
+		content := string(header) + "\nCopyright notice here"
+
+		f, _ := afero.TempFile(AppFs, tempDir, "")


Several subtests silently discard errors with _, inconsistent with require.NoError used elsewhere:

If afero.TempFile fails, f is nil, and f.Name() panics. These should use require.NoError.

CreatorHead · 2026-06-12T07:56:15Z

+		content := string(header) + "\nCopyright notice here"
+
+		f, _ := afero.TempFile(AppFs, tempDir, "")
+		_ = afero.WriteFile(AppFs, f.Name(), []byte(content), 0644)


For consistency we should use require.NoError.

CreatorHead · 2026-06-12T07:57:18Z

+		assert.Nil(t, err)
+
+		// Read file and verify header was added
+		content, _ := afero.ReadFile(AppFs, filePath)


For consistency we should use require.NoError?

CreatorHead · 2026-06-12T07:57:27Z

+		assert.Nil(t, err)
+
+		// Read file and verify header was prepended
+		content, _ := afero.ReadFile(AppFs, filePath)


For consistency we should use require.NoError

CreatorHead · 2026-06-12T07:57:55Z

+		err := AddHeader(filePath, header)
+		assert.Nil(t, err)
+
+		content, _ := afero.ReadFile(AppFs, filePath)


same as above comments

CreatorHead · 2026-06-12T08:00:16Z

+			fileName := "test.go"
+			if tt.name == "Skip .copywrite.hcl file" {
+				fileName = ".copywrite.hcl"
+			}


Please check, I think this is an anti-pattern design? (CMIIW) :
The file name shouldn't be derived from tt.name, if the test name is ever reworded or typo'd, this silently breaks the test case it's meant to cover without any compile-time or runtime signal?

CreatorHead · 2026-06-12T08:02:59Z

+		once = sync.Once{}
+		err := InitializeGitCache(evalRoot)


These subtests directly reset shared package variables like once, firstCommitYearCached, and lastCommitYearsCache. What happens if these tests ever run in parallel?

CreatorHead · 2026-06-12T08:04:08Z

+
+	t.Run("file larger than 300 bytes with copyright after header", func(t *testing.T) {
+		// Create content > 300 bytes with copyright appearing after byte 300
+		header := make([]byte, 350)


Magic number embedded in test assertion?

CreatorHead · 2026-06-12T08:08:49Z

+		assert.Equal(t, 2023, lastCommitYearsCache["test2.txt"])
+	})
+
+	t.Run("InitializeGitCache - Failure", func(t *testing.T) {


The test name says "Failure" but asserts NoError. it seems to be testing that the function swallows errors gracefully. If that's intentional, rename the subtest to reflect the expected behavior, and add a comment explaining why errors are suppressed here.

CreatorHead · 2026-06-12T08:32:36Z

+		// Header should come before original content
+		headerIdx := lo.IndexOf([]byte(string(content)), []byte(header)[0])
+		contentIdx := lo.IndexOf([]byte(string(content)), []byte(originalContent)[0])
+		assert.Less(t, headerIdx, contentIdx)


This is still not fixed — assert.Contains with "C" as the header still passes since "C" exists in the original content.

Add test cases to increase coverage for licensecheck and repodata dir…

5bde509

…ectories

mohanmanikanta2299 requested a review from a team as a code owner June 3, 2026 09:40

Lint error fix.

e2a82ef

CreatorHead reviewed Jun 3, 2026

View reviewed changes

CreatorHead requested changes Jun 3, 2026

View reviewed changes

mohanmanikanta2299 marked this pull request as draft June 4, 2026 05:04

mohanmanikanta2299 requested a review from Copilot June 4, 2026 05:04

Copilot started reviewing on behalf of mohanmanikanta2299 June 4, 2026 05:05 View session

Copilot AI reviewed Jun 4, 2026

Copilot stopped reviewing on behalf of mohanmanikanta2299 due to an error June 4, 2026 05:06
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

mohanmanikanta2299 requested a review from Copilot June 4, 2026 05:09

Copilot started reviewing on behalf of mohanmanikanta2299 June 4, 2026 05:09 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread licensecheck/update_test.go Outdated

Comment thread licensecheck/update_test.go Outdated

Comment thread licensecheck/update_test.go Outdated

Comment thread licensecheck/licensecheck_test.go Outdated

Comment thread repodata/repodata_test.go Outdated

Address co-pilot comments.

c71ba83

mohanmanikanta2299 marked this pull request as ready for review June 4, 2026 06:17

mohanmanikanta2299 marked this pull request as draft June 4, 2026 06:21

mohanmanikanta2299 changed the title ~~Test Coverage Improvement - 1~~ Test Coverage PR-1: Tests added in ./licensecheck and ./repodata subdir Jun 5, 2026

mohanmanikanta2299 marked this pull request as ready for review June 5, 2026 15:49

mohanmanikanta2299 requested review from a team and CreatorHead June 5, 2026 15:49

CreatorHead reviewed Jun 12, 2026

View reviewed changes

CreatorHead requested changes Jun 12, 2026

View reviewed changes

Conversation

mohanmanikanta2299 commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠️ Description

Summary

Changes Made

Testing

🔗 External Links

👍 Definition of Done

🤔 Can be merged upon approval?

PCI review checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CreatorHead Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CreatorHead Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CreatorHead Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mohanmanikanta2299 commented Jun 3, 2026 •

edited

Loading

CreatorHead Jun 12, 2026 •

edited

Loading

CreatorHead Jun 12, 2026 •

edited

Loading

CreatorHead Jun 12, 2026 •

edited

Loading