Used Claude Code to increase test coverage#82
Open
HarryStrandJamf wants to merge 10 commits intomainfrom
Open
Conversation
team-adder
bot
requested review from
BIG-RAT,
KapnKerk,
mjKosmic and
rydgecrakerjamf
There was a problem hiding this comment.
Pull request overview
Adds a broad set of unit tests across Utility/Model/Multipart components and makes small refactors to improve testability (notably TemporaryFileManager and test mocks).
Changes:
- Add new unit tests for XML error parsing, temporary file management, file hashing, command-line processing, upload time formatting, and multiple model behaviors.
- Refine test doubles (
MockFileManager,MockDistributionPoint) to better emulate production behavior during tests. - Refactor
TemporaryFileManagerto supportFileManagerinjection for testability; add repo “agent/assistant” guidance files.
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| JamfSyncTests/Utility/XmlErrorParserTests.swift | Adds unit tests for XmlErrorParser behavior. |
| JamfSyncTests/Utility/TemporaryFileManagerTests.swift | Adds unit tests validating temp directory creation/moves/cleanup via a mock file manager. |
| JamfSyncTests/Utility/FileManagerMoveRetainingPermissionsTests.swift | Adds integration-style tests around FileManager.moveRetainingDestinationPermisssions. |
| JamfSyncTests/Utility/FileHashTests.swift | Adds tests for SHA-512 hashing and Data.hexEncodedString(). |
| JamfSyncTests/Utility/CommandLineProcessingTests.swift | Adds tests for command-line sync setup and argument combinations using mock data model/persistence. |
| JamfSyncTests/Multipart/UploadTimeTests.swift | Adds tests for formatted elapsed time strings. |
| JamfSyncTests/Model/SynchronizeTaskTests.swift | Adds tests for orchestration logic across sync steps and error paths. |
| JamfSyncTests/Model/Jcds2DpTests.swift | Adds tests for JCDS file list retrieval/parsing and failure paths. |
| JamfSyncTests/Model/FolderDpTests.swift | Adds tests for local folder DP listing/deletion/transfer behavior. |
| JamfSyncTests/Model/FileShareDpTests.swift | Adjusts an existing test to better control fileExists behavior via the mock. |
| JamfSyncTests/Model/DistributionPointTests.swift | Adds tests for size conversion helpers and modifies transfer-local-files assertions (currently includes debug output). |
| JamfSyncTests/Mocks/MockFileManager.swift | Extends mock with a configurable fileExists(atPath:isDirectory:) provider. |
| JamfSyncTests/Mocks/MockDistributionPoint.swift | Updates mock transfer behavior to advance SynchronizationProgress like production. |
| JamfSync/Utility/TemporaryFileManager.swift | Injects FileManager for testability and uses it for existence checks. |
| CLAUDE.md | Adds AI-assistant usage guidance for the repo. |
| AGENTS.md | Adds repository structure/style guidelines. |
| .claude/settings.local.json | Adds Claude local settings (contains machine-specific paths). |
Comments suppressed due to low confidence (2)
JamfSyncTests/Model/FileShareDpTests.swift:413
- The
fileExistsResponseProvidersetup here won’t affect the.zipexistence check performed byDistributionPoint.isAcceptableForDp, because that code callsfileExists(atPath:)(withoutisDirectory:). Unless the mock’sfileExists(atPath:)also consultsfileExistsResponseProvider, this test can still filter out.pkgfiles unexpectedly. Fix by updatingMockFileManager.fileExists(atPath:)to delegate to the provider (or adjust the test to configurefileExistsResponseappropriately).
JamfSyncTests/Model/DistributionPointTests.swift:667 - This test includes debug
print(...)statements and comments out the original log assertion. Printing in unit tests creates noisy output and the commented assertion suggests the test is in a temporary/debug state. Please remove the prints and re-enable an assertion that verifies the intended behavior (e.g., expected log message) so failures are actionable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+110
to
+121
| // Calculate expected hash for empty data (what happens when stream has no bytes) | ||
| let expectedHash = SHA512.hash(data: Data()) | ||
| let expectedHashString = Data(expectedHash).hexEncodedString() | ||
|
|
||
| // When | ||
| let result = try await fileHash.createSHA512Hash(filePath: nonExistentFile.path) | ||
|
|
||
| // Then | ||
| // InputStream(fileAtPath:) doesn't return nil for non-existent files, | ||
| // it returns a stream with no bytes, which hashes as empty data | ||
| XCTAssertNotNil(result) | ||
| XCTAssertEqual(result, expectedHashString, "Non-existent file should hash as empty data") |
Comment on lines
66
to
+80
| override func fileExists(atPath path: String) -> Bool { | ||
| return fileExistsResponse | ||
| } | ||
|
|
||
| override func fileExists(atPath path: String, isDirectory: UnsafeMutablePointer<ObjCBool>?) -> Bool { | ||
| if let provider = fileExistsResponseProvider { | ||
| return provider(path, isDirectory) | ||
| } | ||
|
|
||
| // Default behavior | ||
| if let isDirectory { | ||
| isDirectory.pointee = false | ||
| } | ||
| return fileExistsResponse | ||
| } |
Comment on lines
+1
to
+9
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(xcodebuild -project \"/Users/admin/Documents/core/JamfSync/Jamf Sync.xcodeproj\" -list)", | ||
| "Bash(cd \"/Users/admin/Documents/core/JamfSync\" && xcodebuild test -project \"Jamf Sync.xcodeproj\" -scheme \"Jamf Sync\" -destination 'platform=macOS' -enableCodeCoverage YES 2>&1 | tee /tmp/test_output.log)", | ||
| "Bash(xcrun xccov:*)" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah be careful not to leak private info in AI confs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.