-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add unit tests to common #85
Conversation
A few changes were required to get unit tests to work. Mostly: - Move GenerateContentRequest validation from the model to the APIController. It's better to centralize this logic - Throw exceptions at the APIController level rather than model Other changes were kept at a minimum to simplify review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but otherwise looks good! Great job!
common/src/main/kotlin/com/google/ai/client/generativeai/common/APIController.kt
Show resolved
Hide resolved
common/src/main/kotlin/com/google/ai/client/generativeai/common/APIController.kt
Show resolved
Hide resolved
common/src/main/kotlin/com/google/ai/client/generativeai/common/APIController.kt
Show resolved
Hide resolved
common/src/test/java/com/google/ai/client/generativeai/common/StreamingSnapshotTests.kt
Outdated
Show resolved
Hide resolved
val responseList = responses.toList() | ||
responseList.any { | ||
it.candidates?.any { it.citationMetadata?.citationSources?.isNotEmpty() ?: false } | ||
?: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've used this structure of any
at least 7 times in your tests. Tbh, I'm surprised it's not already apart of the stdlib, but might I suggest:
internal fun <T> Iterable<T>?.any(predicate: (T) -> Boolean): Boolean =
this?.let { it.any { predicate(it) } } ?: false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does sound like a great idea... but it triggers a stackoverflow due to recursion
Caused by: java.lang.StackOverflowError
at com.google.ai.client.generativeai.common.util.KotlinKt.any(kotlin.kt:38)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, this should fix it:
internal fun <T> Iterable<T>?.any(predicate: (T) -> Boolean): Boolean =
this?.firstNotNullOfOrNull(predicate) != null
No description provided.