Skip to content
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 Integration Tests for Content Caching #171

Merged
merged 15 commits into from
Jun 17, 2024

Conversation

DellaBitta
Copy link
Collaborator

@DellaBitta DellaBitta commented Jun 13, 2024

Updates to the Content Caching feature in development.

  • adds integration tests
  • adds display name to the creation of content caching.
  • fixes the use of a context cache in the sample implementation.

Documentation PR is separate: #172

The getGenerativeModelFromCachedContent parameter `cache` was errantly wrapped in an object.
No display name yet.
* Integration tests against live backend.
*/

describe("cacheContent", function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only have positive test cases. Is it worth testing cases where we expect errors? For example, testing that we get an error if we pass an invalid cached content name with blank spaces, special characters, empty string, etc...
Example in Python SDK: https://github.com/google-gemini/generative-ai-python/pull/355/files#diff-e57945cd50529ced9f0b8aa12022ab5140a6c706d74ecfd05280778f707d3d36R161-R177

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this offline. There are some unit tests already for negative scenarios where we test the SDK behavior itself. I will work to add some purposely failing integration tests, too, but that shouldn't block this release.

packages/main/test-integration/node/cache-content.test.ts Outdated Show resolved Hide resolved
@DellaBitta DellaBitta marked this pull request as ready for review June 14, 2024 20:28
@@ -53,6 +53,7 @@ describe("GoogleAICacheManager", () => {
systemInstruction: "talk like a cat",
tools: [{ functionDeclarations: [{ name: "myFn" }] }],
toolConfig: { functionCallingConfig: {} },
displayName: "a display name.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If adding it to the call, you should probably add an expectation that it's been added to the body, otherwise no reason to add it here (other than to see there's no error). See the other requestBody expectations below.

Copy link
Collaborator Author

@DellaBitta DellaBitta Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated:

  • now tests that displayName is in the request.
  • added model as a constant at the top of the test, and use the correct model name for launch. (I know it doesn't matter what the model name is, but it's good to be consistent.)

@@ -49,9 +49,7 @@ async function run() {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess can you add a TODO or reminder to update line 41 here with the new official model name along with where you're going to do it in the integration test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added it in at this point. Done!

@DellaBitta DellaBitta requested a review from hsubox76 June 17, 2024 19:23
@DellaBitta DellaBitta merged commit 39a4f8d into ch-caching Jun 17, 2024
3 of 4 checks passed
@DellaBitta DellaBitta deleted the ddb-cachedcontent-testing branch June 17, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants