Bug fixes, unit tests, and SwiftArgumentParser migration#31
Conversation
- Replace broken integration test with 6 comprehensive unit tests - Test initialization, Comparable, encoding/decoding, and array operations - All tests passing This fixes the issue where 'swift test' was running 0 tests
Breaking changes: - Migrate from positional arguments to named flags Old: ackgen path/to/file.plist 1 "Title" New: ackgen --output path/to/file.plist --settings --title "Title" - Add --help and --version commands (auto-generated) Bug fixes: - Fix force unwrap crash when LICENSE files contain invalid UTF-8 - Add proper exit codes (Darwin.exit) so Xcode build phases fail on errors - Replace error returns with ValidationError throws for better error messages Dependencies: - Add swift-argument-parser 1.3.0 Updated Example scripts to use new CLI syntax. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
Throws ValidationError when no license files are found, preventing silent generation of empty plist files. Includes helpful message directing users to resolve SPM packages.
* Add tests for Acknowledgement.all() with fixture plist - Create test fixtures directory with sample plist and invalid UTF-8 file - Add AcknowledgementAllTests with 4 tests covering: - Decoding from fixture plist - Case-insensitive sorting behavior - Graceful handling of missing plist (returns empty array) - Graceful handling of invalid data (returns empty array) - Update Package.swift to include test resources * Update Tests/AckGenTests/AcknowledgementAllTests.swift Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Test invalid UTF-8 plist handling in Acknowledgement.all() (#34) * Initial plan * Rename invalid-utf8-license to .plist and refactor test to actually test it Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> * Remove test helper in favor of explicit path construction (#35) * Initial plan * Remove helper method and make path construction explicit in tests Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com>
d7ef9d3 to
a0ea75c
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes the AckGen CLI by migrating to SwiftArgumentParser, improves failure handling in the license-scanning flow, and adds a fixture-based XCTest suite for AckGenCore.
Changes:
- Refactors the
ackgenexecutable to a SwiftArgumentParserParsableCommandwith--output/--settings/--titleoptions and improved validation/errors. - Adds unit tests + SwiftPM test resources (Fixtures) to validate
Acknowledgementcoding andAcknowledgement.all()behavior. - Updates example shell scripts to use the new flag-based CLI invocation.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
Sources/AckGenCLI/AckGen.swift |
Migrates CLI entrypoint to SwiftArgumentParser; adds UTF-8 handling and validation failures. |
Sources/AckGenCLI/main.swift |
Removes obsolete manual main entrypoint (now provided by @main). |
Package.swift |
Adds swift-argument-parser dependency and configures test resources. |
Package.resolved |
Pins SwiftPM dependency resolution for swift-argument-parser. |
Tests/AckGenTests/AcknowledgementTests.swift |
Adds unit tests for core model init/comparable/coding/encoding. |
Tests/AckGenTests/AcknowledgementAllTests.swift |
Adds fixture-based tests for Acknowledgement.all() decoding/sorting/error paths. |
Tests/AckGenTests/Fixtures/Acknowledgements.plist |
Adds a valid fixture plist for decoding tests. |
Tests/AckGenTests/Fixtures/invalid-utf8-license.plist |
Adds a malformed fixture used to validate decode failure handling. |
Example/ackgen.sh |
Updates example invocation to new flag-based syntax. |
Example/ackgen_settings.sh |
Updates Settings.bundle example invocation to new flag-based syntax. |
CLAUDE.md |
Adds repo guidance doc (currently contains outdated CLI/dependency statements). |
docs/improvement-plan.md |
Adds an internal improvement plan document (contains some examples now inconsistent with current behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func testInitialization() { | ||
| let ack = Acknowledgement(title: "TestPackage", license: "MIT License") | ||
| XCTAssertEqual(ack.title, "TestPackage") | ||
| XCTAssertEqual(ack.license, "MIT License") | ||
| XCTAssertEqual(ack.type, "PSGroupSpecifier") | ||
| } | ||
|
|
||
| func testComparable() { | ||
| let ackA = Acknowledgement(title: "A", license: "") | ||
| let ackB = Acknowledgement(title: "B", license: "") | ||
| let ackZ = Acknowledgement(title: "Z", license: "") | ||
|
|
||
| XCTAssertTrue(ackA < ackB) | ||
| XCTAssertTrue(ackB < ackZ) | ||
| XCTAssertFalse(ackB < ackA) | ||
| } | ||
|
|
||
| func testSortingIsCaseSensitive() { | ||
| // Note: Comparable implementation uses case-sensitive sorting | ||
| // This differs from Acknowledgement.all() which uses case-insensitive | ||
| let acks = [ | ||
| Acknowledgement(title: "Zebra", license: "License 1"), | ||
| Acknowledgement(title: "apple", license: "License 2"), | ||
| Acknowledgement(title: "Banana", license: "License 3") | ||
| ] | ||
|
|
||
| let sorted = acks.sorted() | ||
|
|
||
| // Case-sensitive: uppercase comes before lowercase in ASCII | ||
| XCTAssertEqual(sorted[0].title, "Banana") | ||
| XCTAssertEqual(sorted[1].title, "Zebra") | ||
| XCTAssertEqual(sorted[2].title, "apple") | ||
| } | ||
|
|
||
| func testCodingKeys() { | ||
| let ack = Acknowledgement(title: "TestPackage", license: "MIT License") | ||
| let encoder = PropertyListEncoder() | ||
| encoder.outputFormat = .xml | ||
|
|
||
| XCTAssertNoThrow(try encoder.encode(ack)) | ||
|
|
||
| let data = try! encoder.encode(ack) | ||
| let plistString = String(data: data, encoding: .utf8)! | ||
|
|
||
| // Verify the coding keys are correct (Title, FooterText, Type) | ||
| XCTAssertTrue(plistString.contains("<key>Title</key>")) | ||
| XCTAssertTrue(plistString.contains("<key>FooterText</key>")) | ||
| XCTAssertTrue(plistString.contains("<key>Type</key>")) | ||
| XCTAssertTrue(plistString.contains("<string>PSGroupSpecifier</string>")) | ||
| } | ||
|
|
||
| func testEncodingAndDecoding() throws { | ||
| let original = Acknowledgement(title: "SwiftUI", license: "Apache 2.0") | ||
| let encoder = PropertyListEncoder() | ||
| let data = try encoder.encode(original) | ||
|
|
||
| let decoder = PropertyListDecoder() | ||
| let decoded = try decoder.decode(Acknowledgement.self, from: data) | ||
|
|
||
| XCTAssertEqual(decoded.title, original.title) | ||
| XCTAssertEqual(decoded.license, original.license) | ||
| XCTAssertEqual(decoded.type, original.type) | ||
| } | ||
|
|
||
| func testArrayEncoding() throws { | ||
| let acks = [ | ||
| Acknowledgement(title: "Package1", license: "MIT"), | ||
| Acknowledgement(title: "Package2", license: "Apache") | ||
| ] | ||
|
|
||
| let encoder = PropertyListEncoder() | ||
| encoder.outputFormat = .xml | ||
| let data = try encoder.encode(acks) | ||
|
|
||
| let decoder = PropertyListDecoder() | ||
| let decoded = try decoder.decode([Acknowledgement].self, from: data) | ||
|
|
||
| XCTAssertEqual(decoded.count, 2) | ||
| XCTAssertEqual(decoded[0].title, "Package1") | ||
| XCTAssertEqual(decoded[1].title, "Package2") | ||
| } |
There was a problem hiding this comment.
AcknowledgementTests duplicates coverage that already exists in Tests/AckGenTests/AckGenTests.swift (init/codable/comparable/coding keys). Consider consolidating into one test file to avoid redundant tests and conflicting expectations over time.
| func testInitialization() { | |
| let ack = Acknowledgement(title: "TestPackage", license: "MIT License") | |
| XCTAssertEqual(ack.title, "TestPackage") | |
| XCTAssertEqual(ack.license, "MIT License") | |
| XCTAssertEqual(ack.type, "PSGroupSpecifier") | |
| } | |
| func testComparable() { | |
| let ackA = Acknowledgement(title: "A", license: "") | |
| let ackB = Acknowledgement(title: "B", license: "") | |
| let ackZ = Acknowledgement(title: "Z", license: "") | |
| XCTAssertTrue(ackA < ackB) | |
| XCTAssertTrue(ackB < ackZ) | |
| XCTAssertFalse(ackB < ackA) | |
| } | |
| func testSortingIsCaseSensitive() { | |
| // Note: Comparable implementation uses case-sensitive sorting | |
| // This differs from Acknowledgement.all() which uses case-insensitive | |
| let acks = [ | |
| Acknowledgement(title: "Zebra", license: "License 1"), | |
| Acknowledgement(title: "apple", license: "License 2"), | |
| Acknowledgement(title: "Banana", license: "License 3") | |
| ] | |
| let sorted = acks.sorted() | |
| // Case-sensitive: uppercase comes before lowercase in ASCII | |
| XCTAssertEqual(sorted[0].title, "Banana") | |
| XCTAssertEqual(sorted[1].title, "Zebra") | |
| XCTAssertEqual(sorted[2].title, "apple") | |
| } | |
| func testCodingKeys() { | |
| let ack = Acknowledgement(title: "TestPackage", license: "MIT License") | |
| let encoder = PropertyListEncoder() | |
| encoder.outputFormat = .xml | |
| XCTAssertNoThrow(try encoder.encode(ack)) | |
| let data = try! encoder.encode(ack) | |
| let plistString = String(data: data, encoding: .utf8)! | |
| // Verify the coding keys are correct (Title, FooterText, Type) | |
| XCTAssertTrue(plistString.contains("<key>Title</key>")) | |
| XCTAssertTrue(plistString.contains("<key>FooterText</key>")) | |
| XCTAssertTrue(plistString.contains("<key>Type</key>")) | |
| XCTAssertTrue(plistString.contains("<string>PSGroupSpecifier</string>")) | |
| } | |
| func testEncodingAndDecoding() throws { | |
| let original = Acknowledgement(title: "SwiftUI", license: "Apache 2.0") | |
| let encoder = PropertyListEncoder() | |
| let data = try encoder.encode(original) | |
| let decoder = PropertyListDecoder() | |
| let decoded = try decoder.decode(Acknowledgement.self, from: data) | |
| XCTAssertEqual(decoded.title, original.title) | |
| XCTAssertEqual(decoded.license, original.license) | |
| XCTAssertEqual(decoded.type, original.type) | |
| } | |
| func testArrayEncoding() throws { | |
| let acks = [ | |
| Acknowledgement(title: "Package1", license: "MIT"), | |
| Acknowledgement(title: "Package2", license: "Apache") | |
| ] | |
| let encoder = PropertyListEncoder() | |
| encoder.outputFormat = .xml | |
| let data = try encoder.encode(acks) | |
| let decoder = PropertyListDecoder() | |
| let decoded = try decoder.decode([Acknowledgement].self, from: data) | |
| XCTAssertEqual(decoded.count, 2) | |
| XCTAssertEqual(decoded[0].title, "Package1") | |
| XCTAssertEqual(decoded[1].title, "Package2") | |
| } | |
| // Tests for Acknowledgement (init/codable/comparable/coding keys) | |
| // have been consolidated into AckGenTests.swift to avoid | |
| // redundant coverage and conflicting expectations over time. |
| Three-module Swift Package with no external dependencies: | ||
|
|
There was a problem hiding this comment.
CLAUDE.md says the package has "no external dependencies", but Package.swift now adds swift-argument-parser. Update this statement (and optionally mention the new dependency and why it exists) to keep the architecture overview accurate.
| @main | ||
| struct AckGen: ParsableCommand { | ||
| static let configuration = CommandConfiguration( | ||
| abstract: "Generate acknowledgements plist from Swift Package Manager dependencies", | ||
| version: "0.8.0" | ||
| ) | ||
|
|
||
| @Option(name: .shortAndLong, help: "Output path for the generated plist file") | ||
| var output: String? | ||
|
|
||
| @Flag(name: .long, help: "Generate Settings.bundle format") | ||
| var settings: Bool = false | ||
|
|
||
| static func main() { | ||
| @Option(name: .long, help: "Title for Settings.bundle (only used with --settings)") | ||
| var title: String = "Acknowledgements" | ||
|
|
There was a problem hiding this comment.
PR description mentions updating README CLI usage examples, but README.md in this branch still shows swift run ackgen and positional arguments. Please update README.md to reflect the new flag-based interface (and note the breaking change) so end users don’t follow outdated instructions.
| let packageDirectories = try fman.contentsOfDirectory(atPath: packageCachePath) | ||
| var acknowledgements = [Acknowledgement]() | ||
|
|
||
| let packageDirectories = try fman.contentsOfDirectory(atPath: packageCachePath) | ||
| var acknowledgements = [Acknowledgement]() | ||
|
|
||
| for pkgDir in packageDirectories where pkgDir.prefix(1) != "." { | ||
| for file in licenseFiles { | ||
| guard let data = fman.contents(atPath: "\(packageCachePath)/\(pkgDir)/\(file)") else { continue } | ||
| let new = Acknowledgement(title: pkgDir, license: String(data: data, encoding: .utf8)!) | ||
| acknowledgements.append(new) | ||
| for pkgDir in packageDirectories where pkgDir.prefix(1) != "." { | ||
| for file in licenseFiles { | ||
| guard let data = fman.contents(atPath: "\(packageCachePath)/\(pkgDir)/\(file)") else { continue } | ||
| guard let license = String(data: data, encoding: .utf8) else { | ||
| print("warning: Skipping \(pkgDir)/\(file) - invalid UTF-8 encoding") | ||
| continue | ||
| } | ||
| let new = Acknowledgement(title: pkgDir, license: license) | ||
| acknowledgements.append(new) | ||
| } | ||
| } | ||
|
|
||
| if acknowledgements.isEmpty { | ||
| throw ValidationError( | ||
| "No license files found in \(packageCachePath). " + | ||
| "Ensure SPM packages are resolved (Xcode → File → Packages → Resolve)." | ||
| ) | ||
| } |
There was a problem hiding this comment.
The new CLI behaviors (skipping invalid UTF-8 license files with a warning, and failing with ValidationError when no licenses are found) aren’t covered by tests. Since the repo now has a working XCTest suite, consider adding at least a minimal CLI test (or extracting the scanning/encoding logic into AckGenCore for unit testing) to prevent regressions in build-phase behavior.
|
@MartinP7r I've opened a new pull request, #38, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@MartinP7r I've opened a new pull request, #39, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Architecture | ||
|
|
||
| Three-module Swift Package with no external dependencies: |
There was a problem hiding this comment.
This section states the package has "no external dependencies", but Package.swift now depends on swift-argument-parser. Please update the architecture notes to reflect the new dependency to keep this guidance accurate.
| Three-module Swift Package with no external dependencies: | |
| Three-module Swift Package with one external dependency (`swift-argument-parser` for the CLI module): |
| func testSortingIsCaseInsensitive() { | ||
| let acks = [ | ||
| Acknowledgement(title: "Zebra", license: ""), | ||
| Acknowledgement(title: "apple", license: ""), | ||
| Acknowledgement(title: "Banana", license: "") | ||
| ] | ||
| let sorted = acks.sorted() | ||
| XCTAssertEqual(sorted.map(\.title), ["apple", "Banana", "Zebra"]) | ||
| } |
There was a problem hiding this comment.
This plan’s sample test expects acks.sorted() to produce case-insensitive ordering, but Acknowledgement’s current Comparable implementation compares titles case-sensitively. Either adjust this example to match current behavior, or explicitly note that this is a desired future change and link it to the corresponding code change.
| | File | Change | | ||
| |------|--------| | ||
| | `Package.swift` | Add swift-argument-parser dependency | | ||
| | `Sources/AckGenCLI/AckGen.swift` | Fix crash, add exit codes, migrate to ArgumentParser | | ||
| | `Sources/AckGenCLI/main.swift` | Delete | | ||
| | `Tests/AckGenTests/AckGenTests.swift` | Replace with proper unit tests | | ||
| | `Example/ackgen.sh` | Update CLI invocation | | ||
| | `Example/ackgen_settings.sh` | Update CLI invocation | | ||
| | `README.md` | Update CLI usage examples | |
There was a problem hiding this comment.
This markdown table uses || at the start of each row, which doesn’t render as a standard GitHub-flavored markdown table (it creates an empty first column). Use a single leading | per row (e.g., | File | Change |).
| func testAllReturnsEmptyArrayForInvalidPlist() { | ||
| // Given: A malformed/invalid plist file (fixture: invalid-utf8-license) | ||
| // Note: This test verifies graceful handling of decode failures for malformed plist data | ||
| // The all() method should return an empty array for any plist decode failure | ||
|
|
||
| // When: Attempting to decode a malformed plist that cannot be parsed | ||
| let acks = Acknowledgement.all(fromPlist: "Fixtures/invalid-utf8-license", in: Bundle.module) | ||
|
|
There was a problem hiding this comment.
This test (and the fixture name) describes "invalid UTF-8" behavior, but the code under test (Acknowledgement.all) is decoding a plist resource via PropertyListDecoder. The failure mode here is "invalid plist / decode failure", not UTF-8 license decoding. Consider renaming the fixture/test description to reflect what’s actually being tested (e.g., invalid plist) to avoid confusion with the CLI UTF-8 guard.
| @@ -0,0 +1 @@ | |||
| �������������������������������� No newline at end of file | |||
There was a problem hiding this comment.
This fixture is named like a plist and referenced as an "invalid UTF-8" case, but it is not a valid property list. If the intent is to test plist decode failure, consider renaming it accordingly (e.g., invalid-plist.plist) and/or adding a short comment in the corresponding test clarifying the purpose.
| let settingsTitle: String = arguments.count > 2 ? arguments[2] : "Acknowledgements" | ||
| let plistPath: String = output ?? "\(srcRoot)/Acknowledgements.plist" | ||
|
|
||
| let packageCachePath = tempDirPath.components(separatedBy: "/Build/")[0] + "/SourcePackages/checkouts" |
There was a problem hiding this comment.
packageCachePath is derived by splitting PROJECT_TEMP_DIR on the literal string "/Build/". This is brittle (e.g., paths containing "Build" elsewhere, or when the substring isn’t present) and can produce an incorrect checkout path. Prefer extracting the DerivedData root by finding the last "/Build/" path component (or using URL path components) and fail with a clear ValidationError if it can’t be determined.
| let packageCachePath = tempDirPath.components(separatedBy: "/Build/")[0] + "/SourcePackages/checkouts" | |
| guard let buildRange = tempDirPath.range(of: "/Build/", options: .backwards) else { | |
| throw ValidationError( | |
| "Could not determine DerivedData root from PROJECT_TEMP_DIR (\(tempDirPath)). " + | |
| "Expected path to contain a '/Build/' component." | |
| ) | |
| } | |
| let derivedDataRoot = String(tempDirPath[..<buildRange.lowerBound]) | |
| let packageCachePath = derivedDataRoot + "/SourcePackages/checkouts" |
| func testSortingIsCaseSensitive() { | ||
| // Note: Comparable implementation uses case-sensitive sorting | ||
| // This differs from Acknowledgement.all() which uses case-insensitive | ||
| let acks = [ | ||
| Acknowledgement(title: "Zebra", license: "License 1"), | ||
| Acknowledgement(title: "apple", license: "License 2"), | ||
| Acknowledgement(title: "Banana", license: "License 3") | ||
| ] | ||
|
|
||
| let sorted = acks.sorted() | ||
|
|
||
| // Case-sensitive: uppercase comes before lowercase in ASCII | ||
| XCTAssertEqual(sorted[0].title, "Banana") | ||
| XCTAssertEqual(sorted[1].title, "Zebra") | ||
| XCTAssertEqual(sorted[2].title, "apple") | ||
| } |
There was a problem hiding this comment.
These tests document that Comparable sorting is case-sensitive, while Acknowledgement.all() applies a case-insensitive sort. This inconsistency can surprise API consumers because sorted() will differ from the ordering returned by all(). Consider making Acknowledgement: Comparable use the same case-insensitive comparison as all() (or have all() rely on sorted()), and update this test accordingly.
| # Run the CLI (requires Xcode environment variables SRCROOT, PROJECT_TEMP_DIR) | ||
| swift run ackgen --output <output_path> --settings <forSettings> --title <settingsTitle> | ||
|
|
There was a problem hiding this comment.
The CLI invocation documented here still uses the old positional-argument interface (swift run ackgen [output_path] [forSettings] [settingsTitle]). Since the CLI was migrated to SwiftArgumentParser, this should be updated to the new flags (--output, --settings, --title) to avoid misleading contributors.
- Update CLI usage examples from positional args to --output/--settings/--title flags - Fix CLAUDE.md claiming "no external dependencies" (now has swift-argument-parser) - Fix --settings flag syntax in CLAUDE.md (it's a flag, not an option) - Mark "Add tests" as done in README TODO list - Document Settings.bundle generation in README
Review SummaryLGTM — solid modernization of the CLI and good test coverage additions. Changes reviewed:
Docs commit pushed: Updated README.md and CLAUDE.md to reflect the new flag-based CLI interface and corrected the "no external dependencies" claim. Remaining non-blocking items for follow-up:
✅ Ready to merge once CI passes. |
…igration The documentation was written before PR #31 migrated to SwiftArgumentParser. Update positional args to named options (--output, --settings, --title). Also fix pre-existing outdated syntax in the custom path example.
…igration The documentation was written before PR #31 migrated to SwiftArgumentParser. Update positional args to named options (--output, --settings, --title). Also fix pre-existing outdated syntax in the custom path example.
…ments (#36) * Initial plan * Add Settings.bundle integration instructions to README Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> * Fix CLI syntax in Settings.bundle docs to match SwiftArgumentParser migration The documentation was written before PR #31 migrated to SwiftArgumentParser. Update positional args to named options (--output, --settings, --title). Also fix pre-existing outdated syntax in the custom path example. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MartinP7r <2669027+MartinP7r@users.noreply.github.com> Co-authored-by: Martin Pfundmair <dev@martinp7r.com>
Summary
This PR fixes critical bugs, establishes a working test suite, and modernizes the CLI with SwiftArgumentParser.
Changes
🐛 Bug Fixes
✅ Testing
swift testnow runs 6 passing tests (previously 0)🎯 CLI Modernization
--helpand--versioncommandsCommits
Testing
Migration Notes
This is a breaking change for existing users. Consider: