Conversation
…session lifecycle - Add SpeechSessionErrorTests to verify all error cases have descriptions, failure reasons, and recovery suggestions - Add file transcription tests for invalid URLs and restricted directory paths - Add voice activation test for isSpeechDetected reset behavior - Add session lifecycle tests for pause/resume when idle
Summary of ChangesHello @rudrankriyam, 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 expands the test coverage for the Highlights
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 expands the test coverage for AuralKit, adding 8 new tests for error localization, file transcription edge cases, voice activation, and session lifecycle management. The new tests are well-written and cover important scenarios. My review includes a few suggestions to refactor some of the new test cases to use more modern and concise patterns from the Swift Testing framework, which will improve code clarity and maintainability.
| do { | ||
| _ = try await session.transcribe(audioFile: httpURL) | ||
| Issue.record("Expected audioFileInvalidURL error") | ||
| } catch let error as SpeechSessionError { | ||
| guard case let .audioFileInvalidURL(url) = error else { | ||
| Issue.record("Unexpected error: \(error)") | ||
| return | ||
| } | ||
| #expect(url == httpURL) | ||
| } catch { | ||
| Issue.record("Unexpected error: \(error)") | ||
| } |
There was a problem hiding this comment.
The test logic can be simplified by using the #expect(throws:) macro from the Testing framework. This makes the test more concise and idiomatic for error validation, improving readability.
await #expect(throws: SpeechSessionError.self) {
_ = try await session.transcribe(audioFile: httpURL)
} catch: { error in
guard case let .audioFileInvalidURL(url) = error else {
Issue.record("Unexpected error type: \(error)")
return
}
#expect(url == httpURL)
}| do { | ||
| _ = try await session.transcribe(audioFile: tempURL, options: restrictedOptions) | ||
| Issue.record("Expected audioFileOutsideAllowedDirectories error") | ||
| } catch let error as SpeechSessionError { | ||
| guard case .audioFileOutsideAllowedDirectories = error else { | ||
| Issue.record("Unexpected error: \(error)") | ||
| return | ||
| } | ||
| } catch { | ||
| Issue.record("Unexpected error: \(error)") | ||
| } |
There was a problem hiding this comment.
Similar to the test above, this do-catch block can be simplified using the #expect(throws:) macro. This improves readability and aligns with modern Swift testing practices.
do {
try await #expect(throws: SpeechSessionError.self) {
_ = try await session.transcribe(audioFile: tempURL, options: restrictedOptions)
} catch: { error in
guard case .audioFileOutsideAllowedDirectories = error else {
Issue.record("Unexpected error type: \(error)")
return
}
}
} catch {
Issue.record("Unexpected error: \(error)")
}| @Test("all error cases have non-empty errorDescription") | ||
| func allErrorsHaveDescription() { | ||
| for error in Self.allErrors { | ||
| let description = error.errorDescription | ||
| #expect(description != nil, "Missing errorDescription for \(error)") | ||
| #expect(description?.isEmpty == false, "Empty errorDescription for \(error)") | ||
| } | ||
| } | ||
|
|
||
| @Test("all error cases have non-empty failureReason") | ||
| func allErrorsHaveFailureReason() { | ||
| for error in Self.allErrors { | ||
| let reason = error.failureReason | ||
| #expect(reason != nil, "Missing failureReason for \(error)") | ||
| #expect(reason?.isEmpty == false, "Empty failureReason for \(error)") | ||
| } | ||
| } | ||
|
|
||
| @Test("all error cases have non-empty recoverySuggestion") | ||
| func allErrorsHaveRecoverySuggestion() { | ||
| for error in Self.allErrors { | ||
| let suggestion = error.recoverySuggestion | ||
| #expect(suggestion != nil, "Missing recoverySuggestion for \(error)") | ||
| #expect(suggestion?.isEmpty == false, "Empty recoverySuggestion for \(error)") | ||
| } | ||
| } |
There was a problem hiding this comment.
These three tests are very similar and contain duplicated logic. You can consolidate them into a single, more maintainable parameterized test using the Testing framework's argument support. This reduces boilerplate and makes it easier to add checks for other localized properties in the future.
| @Test("all error cases have non-empty errorDescription") | |
| func allErrorsHaveDescription() { | |
| for error in Self.allErrors { | |
| let description = error.errorDescription | |
| #expect(description != nil, "Missing errorDescription for \(error)") | |
| #expect(description?.isEmpty == false, "Empty errorDescription for \(error)") | |
| } | |
| } | |
| @Test("all error cases have non-empty failureReason") | |
| func allErrorsHaveFailureReason() { | |
| for error in Self.allErrors { | |
| let reason = error.failureReason | |
| #expect(reason != nil, "Missing failureReason for \(error)") | |
| #expect(reason?.isEmpty == false, "Empty failureReason for \(error)") | |
| } | |
| } | |
| @Test("all error cases have non-empty recoverySuggestion") | |
| func allErrorsHaveRecoverySuggestion() { | |
| for error in Self.allErrors { | |
| let suggestion = error.recoverySuggestion | |
| #expect(suggestion != nil, "Missing recoverySuggestion for \(error)") | |
| #expect(suggestion?.isEmpty == false, "Empty recoverySuggestion for \(error)") | |
| } | |
| } | |
| private struct LocalizedProperty: Sendable { | |
| let name: String | |
| let keyPath: KeyPath<SpeechSessionError, String?> | |
| } | |
| private static let localizedProperties: [LocalizedProperty] = [ | |
| .init(name: "errorDescription", keyPath: \.errorDescription), | |
| .init(name: "failureReason", keyPath: \.failureReason), | |
| .init(name: "recoverySuggestion", keyPath: \.recoverySuggestion) | |
| ] | |
| @Test("all error cases have non-empty localized strings", arguments: localizedProperties) | |
| func allErrorsHaveNonEmptyLocalizedStrings(property: LocalizedProperty) { | |
| for error in Self.allErrors { | |
| let value = error[keyPath: property.keyPath] | |
| #expect(value != nil, "Missing \(property.name) for \(error)") | |
| #expect(value?.isEmpty == false, "Empty \(property.name) for \(error)") | |
| } | |
| } |
1 participant
This PR adds additional test coverage for:
Error Localization (3 tests)
File Transcription Edge Cases (2 tests)
Voice Activation (1 test)
Session Lifecycle (2 tests)
Test count increased from 12 to 20.