Add AppStats unit tests, Swift package target, and README test docs#74
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the KeyStats project by integrating robust unit testing for the AppStats model and establishing a Swift package target for its core logic. These changes ensure the reliability and maintainability of the application's statistics tracking, providing a clear and automated way to validate its behavior across various scenarios, including data persistence and backward compatibility. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for the AppStats model, which is a great step for improving code quality and ensuring correctness. The changes also include setting up a Swift Package to make the code testable via swift test and updating the documentation.
My review focuses on a few areas:
- A critical issue with how the
AppStats.swiftsource file is included in the Swift Package, which will likely prevent it from building. - Improving the robustness of the new tests by avoiding force unwraps and magic numbers.
- Minor convention improvements in
Package.swiftand formatting in the README files.
Overall, the tests are well-written and cover important cases, including legacy data migration. Addressing the feedback will make the implementation more robust and conventional.
| @@ -0,0 +1 @@ | |||
| ../../KeyStats/AppStats.swift No newline at end of file | |||
There was a problem hiding this comment.
This file appears to contain a relative path as its content, rather than being a Swift source file or a symbolic link. The Swift compiler cannot process a text file containing a path, so the package build will fail. To share the AppStats.swift source file between your Xcode project and this Swift package, you should use a symbolic link or adjust the target's path in Package.swift.
Recommended Fix (Symbolic Link):
Delete this text file and create a symlink instead:
ln -s ../../KeyStats/AppStats.swift Sources/KeyStatsCore/AppStats.swiftAlternative Fix (Package.swift):
Alternatively, you could modify your Package.swift to point directly to the source file's directory, though this might require more restructuring.
Given that the PR description states that swift test passes, it's possible this is intended to be a symlink and is being misinterpreted by Git. If so, please ensure it's committed as a symlink.
| XCTAssertEqual(decoded.keyPresses, 12) | ||
| XCTAssertEqual(decoded.leftClicks, 3) | ||
| XCTAssertEqual(decoded.rightClicks, 4) | ||
| XCTAssertEqual(decoded.sideBackClicks, 5) | ||
| XCTAssertEqual(decoded.sideForwardClicks, 6) | ||
| XCTAssertEqual(decoded.scrollDistance, 18.2, accuracy: 0.0001) |
There was a problem hiding this comment.
To improve maintainability and reduce redundancy, it's better to assert against the properties of the original object rather than using hardcoded values. This way, if you change the test data, you only need to update it in one place.
| XCTAssertEqual(decoded.keyPresses, 12) | |
| XCTAssertEqual(decoded.leftClicks, 3) | |
| XCTAssertEqual(decoded.rightClicks, 4) | |
| XCTAssertEqual(decoded.sideBackClicks, 5) | |
| XCTAssertEqual(decoded.sideForwardClicks, 6) | |
| XCTAssertEqual(decoded.scrollDistance, 18.2, accuracy: 0.0001) | |
| XCTAssertEqual(decoded.keyPresses, original.keyPresses) | |
| XCTAssertEqual(decoded.leftClicks, original.leftClicks) | |
| XCTAssertEqual(decoded.rightClicks, original.rightClicks) | |
| XCTAssertEqual(decoded.sideBackClicks, original.sideBackClicks) | |
| XCTAssertEqual(decoded.sideForwardClicks, original.sideForwardClicks) | |
| XCTAssertEqual(decoded.scrollDistance, original.scrollDistance, accuracy: 0.0001) |
| let json = """ | ||
| { | ||
| "bundleId": "com.test.app", | ||
| "displayName": "Test App", | ||
| "keyPresses": 2, | ||
| "leftClicks": 1, | ||
| "rightClicks": 1, | ||
| "otherClicks": 9, | ||
| "scrollDistance": 4.5 | ||
| } | ||
| """.data(using: .utf8)! |
There was a problem hiding this comment.
Using XCTUnwrap is safer than force-unwrapping (!) the result of data(using: .utf8). This prevents the test from crashing if the string cannot be converted to data and instead provides a more informative test failure.
| let json = """ | |
| { | |
| "bundleId": "com.test.app", | |
| "displayName": "Test App", | |
| "keyPresses": 2, | |
| "leftClicks": 1, | |
| "rightClicks": 1, | |
| "otherClicks": 9, | |
| "scrollDistance": 4.5 | |
| } | |
| """.data(using: .utf8)! | |
| let json = try XCTUnwrap(""" | |
| { | |
| "bundleId": "com.test.app", | |
| "displayName": "Test App", | |
| "keyPresses": 2, | |
| "leftClicks": 1, | |
| "rightClicks": 1, | |
| "otherClicks": 9, | |
| "scrollDistance": 4.5 | |
| } | |
| """.data(using: .utf8)) |
| let json = """ | ||
| { | ||
| "bundleId": "com.test.app", | ||
| "displayName": "Test App", | ||
| "leftClicks": 1, | ||
| "rightClicks": 1, | ||
| "sideBackClicks": 2, | ||
| "sideForwardClicks": 3, | ||
| "otherClicks": 100, | ||
| "scrollDistance": 0 | ||
| } | ||
| """.data(using: .utf8)! |
There was a problem hiding this comment.
Using XCTUnwrap is safer than force-unwrapping (!) the result of data(using: .utf8). This prevents the test from crashing if the string cannot be converted to data and instead provides a more informative test failure.
| let json = """ | |
| { | |
| "bundleId": "com.test.app", | |
| "displayName": "Test App", | |
| "leftClicks": 1, | |
| "rightClicks": 1, | |
| "sideBackClicks": 2, | |
| "sideForwardClicks": 3, | |
| "otherClicks": 100, | |
| "scrollDistance": 0 | |
| } | |
| """.data(using: .utf8)! | |
| let json = try XCTUnwrap(""" | |
| { | |
| "bundleId": "com.test.app", | |
| "displayName": "Test App", | |
| "leftClicks": 1, | |
| "rightClicks": 1, | |
| "sideBackClicks": 2, | |
| "sideForwardClicks": 3, | |
| "otherClicks": 100, | |
| "scrollDistance": 0 | |
| } | |
| """.data(using: .utf8)) |
| } | ||
|
|
||
| func testDecodeMissingFieldsFallsBackToZeroAndEmpty() throws { | ||
| let json = "{}".data(using: .utf8)! |
There was a problem hiding this comment.
Using XCTUnwrap is safer than force-unwrapping (!) the result of data(using: .utf8). This prevents the test from crashing if the string cannot be converted to data and instead provides a more informative test failure.
| let json = "{}".data(using: .utf8)! | |
| let json = try XCTUnwrap("{}".data(using: .utf8)) |
| import PackageDescription | ||
|
|
||
| let package = Package( | ||
| name: "KeyStatsCoreTests", |
There was a problem hiding this comment.
It's conventional for the package name to match the primary library or executable it produces. In this case, naming the package KeyStatsCore would be more aligned with standard Swift package practices, as the main product is the KeyStatsCore library.
| name: "KeyStatsCoreTests", | |
| name: "KeyStatsCore", |
| - **UI Mode**: System tray application | ||
| - **Advantages**: No runtime installation required, ready to use on Windows 10/11 out of the box, small app size (5-10 MB) | ||
|
|
||
|
|
There was a problem hiding this comment.
This added blank line creates unnecessary vertical space. Please remove it to improve readability.
| - **UI 模式**：系统托盘应用 | ||
| - **优势**：无需安装运行时，Windows 10/11 开箱即用，应用体积小（5-10 MB） | ||
|
|
||
|
|
There was a problem hiding this comment.
This added blank line creates unnecessary vertical space. Please remove it to improve readability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ec550527
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,21 @@ | |||
| // swift-tools-version:5.9 | |||
There was a problem hiding this comment.
Package.swift now requires swift-tools-version:5.9, which makes swift test fail up front on any toolchain older than Swift 5.9 even though the repository still documents Swift 5.0 as the macOS language baseline (README.md technical implementation section). Since this commit positions swift test as the regression path for AppStats, that unnecessary minimum version blocks contributors using Swift 5.0–5.8 from running the new tests.
Useful? React with 👍 / 👎.
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
@claude review |
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
debugtheworldbot
deleted the
codex/generate-complete-test-cases-and-code
branch
1 participant
Motivation
AppStatsmodel to ensure correct counting, decoding, and update behaviors.AppStatslogic testable viaswift testby exposing the core sources as a package target.Description
KeyStatsTests/AppStatsTests.swiftcontainingXCTestcases that verify initialization defaults, recording methods, scroll accumulation, display name updates, Codable round-trip, legacyotherClicksdecoding behavior, and missing-field fallbacks.Package.swiftto define aKeyStatsCoretarget and aKeyStatsCoreTeststest target that includesAppStatsTests.swift.Sources/KeyStatsCore/AppStats.swiftas a path entry pointing at the existingKeyStats/AppStats.swiftso the package target builds the app model.README.mdandREADME_ZH.mdto document the newAppStatstests and include instructions to run them withswift test.Testing
swift testto execute theAppStatsXCTest suite.KeyStatsTests/AppStatsTests.swiftpassed successfully, including legacy decoding and Codable round-trip checks.KeyStatsCoretarget and its test target without errors when running the test command.Codex Task