Add Crashlytics agent evals #9516
Conversation
schnecle
force-pushed
the
schnecle/add-agent-evals
branch
from
69c0fd3 to
8321bf4
Compare
|
/gemini review |
![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 makes significant improvements by adding e2e tests for Crashlytics, introducing memory interaction and assertion negation for tests, and refactoring agent-evals to a CommonJS library. The use of a shared renderTemplate for mocks is a great change to prevent test drift. My review has identified a few issues. There are some incorrect paths in the test configuration (.mocharc.yml and package.json) and a critical path issue in the GeminiCliRunner that will likely cause tests to fail. I've also noted some debug console.log statements that should be removed and some copy-paste errors in the new template README files. Addressing these points will improve the robustness and clarity of the new testing infrastructure.
schnecle
force-pushed
the
schnecle/add-agent-evals
branch
from
8321bf4 to
e3e7221
Compare
|
|
||
| export const getEnvironmentWithIosApp = { | ||
| firebase_get_environment: toMockContent( | ||
| renderTemplate({ ...BASE_ENVIRONMENT_CONFIG, detectedAppIds: { [IOS_APP_ID]: IOS_BUNDLE_ID } }), |
samedson
left a comment
samedson
left a comment
There was a problem hiding this comment.
Have some questions in there, but excited to get this in!
samedson
left a comment
samedson
left a comment
There was a problem hiding this comment.
LGTM! Just have that one nit on the dirs variable
github-project-automation
bot
moved this to Approved [PR]
in [Cloud] Extensions + Functions
schnecle
force-pushed
the
schnecle/add-agent-evals
branch
from
286aed7 to
f752ee4
Compare
…ional test coverage across supported platforms
schnecle
force-pushed
the
schnecle/add-agent-evals
branch
from
8872903 to
c210cdc
Compare
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
LGTM with some nits and small q's
| await run.type("/crashlytics:connect"); | ||
| await run.expectToolCalls(["firebase_get_environment"]); | ||
|
|
||
| await run.expectText("prioritize"); |
There was a problem hiding this comment.
Mostly for my own understanding - why do we look for the word 'prioritize' here?
There was a problem hiding this comment.
The end of the crashlytics:connect prompt asks the agent to ask the user whether they would like to take either of the following actions --
- Prioritize the most impactful stability issues
- Diagnose and propose a fix for a crash
I'm just making sure that it asks that question.
github-project-automation
bot
moved this from Approved [PR]
to Done
in [Cloud] Extensions + Functions
3 participants
Description
Note: the bulk of the changes are in the template apps which are just small skeleton apps for the purpose of a smoke test for auto detection using the most common cases. The content of those apps is largely irrelevant but leads to quite a few file changes.
Scenarios Tested
Sample Commands
cd scripts/agent-evals
npm run test:dev