ci: add Windows CI and expand test coverage#8
Conversation
Add Windows CI workflow using compnerd/gha-setup-swift with Swift 6.1-RELEASE, matching the pattern from AppState and Fork. The package only depends on Foundation, so it is fully cross-platform compatible. Expand test coverage from 3 to 20 tests covering: - WaitError.errorDescription formatting - Free function timeout paths (closure and Equatable variants) - Immediate success (condition already satisfied) - Negative duration/interval (abs() behavior) - Custom polling intervals - Waitable protocol wait(on:for:) with external objects - Waitable self (AnyObject) default parameter usage - Multiple sequential value changes - Boolean and Optional value types - Task cancellation propagation - Discardable result usage Closes 0xLeif#7 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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 enhances the project's robustness and cross-platform compatibility. It establishes continuous integration for Windows, ensuring the "Waiter" utility functions correctly across different operating systems. Concurrently, it substantially broadens the test suite, validating critical functionalities and edge cases of the "Waiter" API. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
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 significantly improves the project by adding a CI workflow for Windows and greatly expanding the test suite from 3 to 20 tests. The new tests cover a wide range of scenarios, including error conditions, edge cases like negative durations, and different API usage patterns. My review focuses on making the new tests even more robust, particularly in how they verify thrown errors. By checking error properties directly instead of relying on string descriptions, the tests will be more resilient to future changes.
| } catch { | ||
| XCTAssertTrue(error is WaitError) | ||
| XCTAssertEqual( | ||
| error.localizedDescription, | ||
| WaitError.timeout(0.3).localizedDescription | ||
| ) | ||
| } |
There was a problem hiding this comment.
Asserting against localizedDescription can make tests brittle, as the string could change (e.g., with localization). A more robust approach is to inspect the error's properties directly. You can cast the error to WaitError and check its case and associated value.
| } catch { | |
| XCTAssertTrue(error is WaitError) | |
| XCTAssertEqual( | |
| error.localizedDescription, | |
| WaitError.timeout(0.3).localizedDescription | |
| ) | |
| } | |
| } catch { | |
| if let waitError = error as? WaitError, case .timeout(let duration) = waitError { | |
| XCTAssertEqual(duration, 0.3) | |
| } else { | |
| XCTFail("Expected WaitError.timeout(0.3), but got \(error)") | |
| } | |
| } |
| } catch { | ||
| XCTAssertTrue(error is WaitError) | ||
| } |
There was a problem hiding this comment.
This error check only verifies the error type. For a more robust test, you should also verify the error's contents, such as the timeout duration. Instead of checking the localizedDescription which can be brittle, it's better to cast the error and inspect its properties directly.
} catch {
if let waitError = error as? WaitError, case .timeout(let duration) = waitError {
XCTAssertEqual(duration, 0.3)
} else {
XCTFail("Expected WaitError.timeout(0.3), but got \(error)")
}
}| } catch { | ||
| // Should timeout at abs(-0.3) = 0.3 seconds | ||
| XCTAssertEqual( | ||
| error.localizedDescription, | ||
| WaitError.timeout(0.3).localizedDescription | ||
| ) | ||
| } |
There was a problem hiding this comment.
Asserting against localizedDescription can make tests brittle, as the string could change (e.g., with localization). A more robust approach is to inspect the error's properties directly. You can cast the error to WaitError and check its case and associated value.
} catch {
// Should timeout at abs(-0.3) = 0.3 seconds
if let waitError = error as? WaitError, case .timeout(let duration) = waitError {
XCTAssertEqual(duration, 0.3)
} else {
XCTFail("Expected WaitError.timeout(0.3), but got \(error)")
}
}| } catch { | ||
| XCTAssertTrue(error is WaitError) | ||
| } |
There was a problem hiding this comment.
This error check only verifies the error type. For a more robust test, you should also verify the error's contents, such as the timeout duration. It's better to cast the error and inspect its properties directly.
} catch {
if let waitError = error as? WaitError, case .timeout(let duration) = waitError {
XCTAssertEqual(duration, 0.3)
} else {
XCTFail("Expected WaitError.timeout(0.3), but got \(error)")
}
}…escription Address review feedback — replace brittle localizedDescription string comparisons and bare type checks with direct pattern matching on WaitError.timeout, verifying the associated duration value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the review! Good call on avoiding Addressed all 4 comments in a99dbd0: replaced |
corvid-agent
left a comment
corvid-agent
left a comment
There was a problem hiding this comment.
Review: Approve (self-authored PR — cannot use GitHub's approve action)
Strong PR — well-structured Windows CI addition and substantial test coverage expansion.
CI:
- Windows workflow follows the established pattern from AppState/Fork using
compnerd/gha-setup-swift@mainwith Swift 6.1-RELEASE. - Triggers on both push (all branches) and pull_request (main) — good.
- The package only uses Foundation, so full cross-platform compatibility is expected.
Test coverage (3 -> 20 tests):
WaitError.errorDescriptiontests verify the user-facing timeout message format.- Free function timeout tests cover both closure and Equatable variants.
- Immediate success tests verify the fast path when conditions are already met.
- Negative duration/interval tests verify the
abs()safety behavior — good defensive testing. Waitableprotocol tests cover bothwait(on:for:)on external objects andself-based defaults.- The task cancellation test is particularly valuable — it verifies that
CancellationErrorpropagates correctly throughTask.sleep. - Multiple value changes test (step 0->1->2->3) verifies polling behavior across state transitions.
- Optional value tests cover nil -> non-nil transitions for both closure and Equatable variants.
All tests use @unchecked Sendable on the Value classes correctly (mutable state accessed from multiple tasks in tests).
LGTM.
|
Review: Clean PR. The Windows workflow is a good addition since the package only depends on Foundation. Test expansion from 3 to 20 is thorough — particularly good to see task cancellation and negative duration edge cases covered. Minor suggestion: CI is green across all three platforms. |
|
Could not retrieve PR diff for security validation This PR requires manual review before merging. |
2 similar comments
|
Could not retrieve PR diff for security validation This PR requires manual review before merging. |
|
Could not retrieve PR diff for security validation This PR requires manual review before merging. |
|
Friendly ping — this PR is ready for review when you have a moment. All CI checks are passing. |
1 participant
Summary
windows.ymlCI workflow usingcompnerd/gha-setup-swift@mainwith Swift 6.1-RELEASE, matching the pattern from AppState and ForkCI Changes
windows.ymlworkflow triggers on push (all branches) and pull_request (main)Test Coverage Added
WaitError.errorDescriptionabs()behavior)Waitable.wait(on:for:)Waitableself default paramsTest plan
Closes #7
🤖 Generated with Claude Code