Add test coverage for duplicate license file prevention#42
Conversation
Verifies that only one acknowledgement is created per package when multiple license file variants exist (LICENSE, LICENSE.txt, LICENSE.md). Closes #39
There was a problem hiding this comment.
Pull request overview
This PR adds test coverage for the break statement in the license scanning loop in AckGen.swift that prevents duplicate Acknowledgement entries when a package directory contains multiple license files (LICENSE, LICENSE.txt, LICENSE.md). Because the scanning logic lives in an executable target (AckGenCLI) that cannot be @testable imported, the tests implement a local copy of the scanning logic with a shouldBreak toggle to simulate both correct and buggy behavior.
Changes:
- Adds
DuplicateLicenseFileTests.swiftwith 3 test cases covering: single-package deduplication, multi-package deduplication with priority ordering, and a regression demonstration of the pre-breakduplicate bug.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try? FileManager.default.removeItem(at: tempDir) | ||
| } | ||
|
|
||
| /// Scans the package cache path for license files, mimicking the CLI logic. |
There was a problem hiding this comment.
The scanForLicenses helper method reimplements the scanning logic from AckGen.run() rather than calling the actual production code. Because AckGenCLI is an executable target (with @main), it cannot be imported into the test target — so the duplication is understandable. However, there is a subtle behavioral divergence: in AckGen.swift (lines 51–54), when a license file's content cannot be decoded as UTF-8, the code prints a warning and continues to try the next license filename. In contrast, the test helper (line 38) simply continues silently without a warning. More critically, both continue in the UTF-8 failure case and then break on success — this means that if the first license file has invalid UTF-8 but a subsequent one is valid, the production code skips the first and appends the second then breaks. The test helper does the same functionally, so the core logic under test is consistent. Still, adding a comment explaining why the logic is duplicated (because AckGenCLI is an executable target) would improve maintainability and prevent future confusion.
| /// Scans the package cache path for license files, mimicking the CLI logic. | |
| /// Scans the package cache path for license files, mimicking the CLI logic. | |
| /// | |
| /// This intentionally re-implements the scanning portion of `AckGen.run()` | |
| /// instead of calling the production CLI code directly, because | |
| /// `AckGenCLI` is an executable target with `@main` and cannot be | |
| /// imported into the test target. | |
| /// | |
| /// The control flow (including `continue` on UTF-8 decode failure and | |
| /// `break` after the first successful match per package) is kept in sync | |
| /// with the CLI implementation. The only intentional difference is that | |
| /// this helper does not print the warning message emitted by the CLI when | |
| /// UTF-8 decoding fails, since logging is not relevant to these tests. |
| // Demonstrates the bug that would occur without break | ||
| let acknowledgements = try scanForLicenses(shouldBreak: false) | ||
|
|
||
| XCTAssertEqual(acknowledgements.count, 2, "Without break, duplicates are created") |
There was a problem hiding this comment.
On line 97, acknowledgements[1] is accessed using a hardcoded index after an XCTAssertEqual count check on line 96. In XCTest, a failing assertion does not stop execution of the test — the test continues past the failing assertion. If acknowledgements.count is not 2 (e.g., it is 0 or 1), accessing acknowledgements[1] will crash the test process with a fatal "index out of range" error rather than producing a clean test failure. The assertion on line 97 should guard against this, for example by only proceeding if the count is large enough, or by using optional subscripting or XCTAssertGreaterThanOrEqual(acknowledgements.count, 2) before the index access.
| XCTAssertEqual(acknowledgements.count, 2, "Without break, duplicates are created") | |
| XCTAssertEqual(acknowledgements.count, 2, "Without break, duplicates are created") | |
| guard acknowledgements.count >= 2 else { | |
| XCTFail("Expected at least 2 acknowledgements to compare titles") | |
| return | |
| } |
|
|
||
| let acknowledgements = try scanForLicenses() | ||
|
|
||
| XCTAssertEqual(acknowledgements.count, 1, "Should only create one acknowledgement per package") |
There was a problem hiding this comment.
On lines 58–59, acknowledgements[0] is accessed after the count assertion on line 57. In XCTest, a failing XCTAssertEqual does not stop test execution, so if the array is unexpectedly empty, the forced index access on these lines will crash the test process with an "index out of range" fatal error instead of producing a clean failure message. Consider guarding with an early return or using XCTAssertGreaterThanOrEqual(acknowledgements.count, 1) followed by a guard before accessing the index.
| XCTAssertEqual(acknowledgements.count, 1, "Should only create one acknowledgement per package") | |
| XCTAssertEqual(acknowledgements.count, 1, "Should only create one acknowledgement per package") | |
| guard acknowledgements.count == 1 else { return } |
Summary
breakbehavior in the license scanning loopSupersedes #39 (which targeted the already-merged
feature/bug-fixes-and-testsbranch).Test plan