Add happy-path test cases for Model and ApiClient #16

natebosch · 2024-01-27T00:26:02Z

Refactor ApiClient to go from Map to Map instead of bytes to
String. This makes it easier to write a stub client which is agnostic
to the ordering of values in the maps. Move the responsibilities for
UTF8 and JSON encoding to the HttpApiClient.

Eagerly call nested .toJson() methods for child fields in .toJson()
implementations. This allows deep equality checking for the stub client
without implementing operator == for the data model classes.

Add a createModelWithClient method in src/model.dart which will not
be exported to the package public API. Use it in the test to create a
model with a stubbed client.

Add tests for each call in GenerativeModel. Tests validate

Encoding from arguments to the JSON schema.
Building the correct Uri for the task and model.
Parsing the result to the Dart data model.

Add tests for unary and streaming calls in HttpClient. Tests validate

Headers are set correctly.
Encoding and decoding UTF8 and JSON.
Using streaming with alt=sse query parameter and parsing of data:
lines in the response.

Add Matcher helper methods for classes checked during tests that don't
have operator ==.

Refactor `ApiClient` to go from `Map` to `Map` instead of bytes to `String`. This makes it easier to write a stub client which is agnostic to the ordering of values in the maps. Move the responsibilities for UTF8 and JSON encoding to the `HttpApiClient`. Eagerly call nested `.toJson()` methods for child fields in `.toJson()` implementations. This allows deep equality checking for the stub client without implementing `operator ==` for the data model classes. Add a `createModelWithClient` method in `src/model.dart` which will not be exported to the package public API. Use it in the test to create a model with a stubbed client. Add tests for each call in `GenerativeModel`. Tests validate - Encoding from arguments to the JSON schema. - Building the correct `Uri` for the task and model. - Parsing the result to the Dart data model. Add tests for unary and streaming calls in `HttpClient`. Tests validate - Headers are set correctly. - Encoding and decoding UTF8 and JSON. - Using streaming with `alt=sse` query parameter and parsing of `data: ` lines in the response. Add Matcher helper methods for classes checked during tests that don't have `operator ==`.

devoncarew

lgtm; great refactoring for testability.

Feel free to ignore the tailing comma suggested edits, though I suspect some of them will help w/ idents and readability in the files.

I would comment back in the dart test check at the end of the google_generative_ai.yml workflow file before landing this.

devoncarew · 2024-01-27T21:32:16Z

pkgs/google_generative_ai/lib/src/client.dart

-        headers: {..._headers, 'Content-Type': 'application/json'}, body: body);
-    return utf8.decode(response.bodyBytes);
+        headers: {..._headers, 'Content-Type': 'application/json'},
+        body: utf8.encode(jsonEncode(body)));


for readability?

Suggested change

body: utf8.encode(jsonEncode(body)));

body: utf8.encode(jsonEncode(body)),);

Yeah it looks better that way. Hopefully soon we won't need to make these choices manually 🤞

devoncarew · 2024-01-27T21:33:57Z

pkgs/google_generative_ai/lib/src/model.dart

+          {required String model,
+          required String apiKey,
+          List<SafetySetting> safetySettings = const [],
+          GenerationConfig? generationConfig}) =>


Suggested change

GenerationConfig? generationConfig}) =>

GenerationConfig? generationConfig},) =>

devoncarew · 2024-01-27T21:36:42Z

pkgs/google_generative_ai/lib/src/model.dart

    return parseEmbedContentReponse(response);
  }
 }
+
+GenerativeModel createModelwithClient(


This is only used for testing; I'm not sure if this method is visible outside this library, but assuming it is, perhaps annotate with visibleForTesting? Good to document this in either case.

I'll add a comment for now. Adding a dependency on package:meta is low-cost - but I don't think it's worth for just this annotation.

devoncarew · 2024-01-27T21:36:51Z

pkgs/google_generative_ai/pubspec.yaml

@@ -10,10 +10,11 @@ environment:
  sdk: ^3.2.0

 dependencies:
-  crypt: ^4.3.0


+1 - nice to lose a dep

devoncarew · 2024-01-27T21:37:01Z

pkgs/google_generative_ai/test/generative_model_test.dart

@@ -0,0 +1,154 @@
+import 'package:google_generative_ai/google_generative_ai.dart';


nit: copyright header

Oops. Done.

devoncarew · 2024-01-27T21:37:26Z

pkgs/google_generative_ai/test/generative_model_test.dart

+                    }
+                  ]
+                }
+            ]);


Suggested change

]);

],);

Good call. Added trailing commas for all these subsequent collection arguments.

pkgs/google_generative_ai/test/http_api_client_test.dart

pkgs/google_generative_ai/test/utils/matchers.dart

pkgs/google_generative_ai/test/utils/stub_client.dart

Trailing commas. Copyright headers. Document testing-only

Refactor `ApiClient` to go from `Map` to `Map` instead of bytes to `String`. This makes it easier to write a stub client which is agnostic to the ordering of values in the maps. Move the responsibilities for UTF8 and JSON encoding to the `HttpApiClient`. Eagerly call nested `.toJson()` methods for child fields in `.toJson()` implementations. This allows deep equality checking for the stub client without implementing `operator ==` for the data model classes. Add a `createModelWithClient` method in `src/model.dart` which will not be exported to the package public API. Use it in the test to create a model with a stubbed client. Add tests for each call in `GenerativeModel`. Tests validate - Encoding from arguments to the JSON schema. - Building the correct `Uri` for the task and model. - Parsing the result to the Dart data model. Add tests for unary and streaming calls in `HttpClient`. Tests validate - Headers are set correctly. - Encoding and decoding UTF8 and JSON. - Using streaming with `alt=sse` query parameter and parsing of `data: ` lines in the response. Add Matcher helper methods for classes checked during tests that don't have `operator ==`.

natebosch requested a review from devoncarew January 27, 2024 00:26

natebosch added 2 commits January 27, 2024 00:44

Merge branch 'main' into happy-path-tests

6194a98

Fix import ordering

76dd374

devoncarew approved these changes Jan 27, 2024

View reviewed changes

natebosch added 3 commits January 30, 2024 01:48

Review comments

50f94e3

Trailing commas. Copyright headers. Document testing-only

Run tests on CI

4f163d2

Merge branch 'main' into happy-path-tests

224d7f2

natebosch merged commit 7863b48 into main Jan 30, 2024
7 checks passed

natebosch deleted the happy-path-tests branch January 30, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add happy-path test cases for Model and ApiClient #16

Add happy-path test cases for Model and ApiClient #16

natebosch commented Jan 27, 2024

devoncarew left a comment

devoncarew Jan 27, 2024

natebosch Jan 30, 2024

devoncarew Jan 27, 2024

natebosch Jan 30, 2024

devoncarew Jan 27, 2024

natebosch Jan 30, 2024

devoncarew Jan 27, 2024

devoncarew Jan 27, 2024

natebosch Jan 30, 2024

devoncarew Jan 27, 2024

natebosch Jan 30, 2024

	body: utf8.encode(jsonEncode(body)));
	body: utf8.encode(jsonEncode(body)),);

	GenerationConfig? generationConfig}) =>
	GenerationConfig? generationConfig},) =>

		@@ -0,0 +1,154 @@
		import 'package:google_generative_ai/google_generative_ai.dart';

Add happy-path test cases for Model and ApiClient #16

Add happy-path test cases for Model and ApiClient #16

Conversation

natebosch commented Jan 27, 2024

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment