Add Vega support #783
Add Vega support #783
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @isarkis, 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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR introduces support for running 'Vega' tests, specifically integrating them into the GitHub Actions CI/CD pipeline as End-to-End (E2E) tests. It achieves this by adding a new composite GitHub Action dedicated to triggering these Vega E2E tests via an existing test gateway client script. Additionally, it updates the existing on-device test action to accept a device type parameter and modifies the Android build configurations (android-arm and android-arm64) to specify the device type and, for android-arm64, enable the new E2E tests and define initial test targets.
Highlights
- New Vega E2E Test Action: A new composite GitHub Action (
.github/actions/e2e_tests/action.yaml) is added to specifically handle the triggering of 'vega_test' type E2E tests using theon_device_tests_gateway_client.pyscript. This action takes inputs for test dimensions, device type, and Vega test targets. - Update to On-Device Test Action: The existing composite GitHub Action for on-device tests (
.github/actions/on_device_tests/action.yaml) is updated to accept atest_e2e_device_typeinput and pass it as the--device_typeargument to the test gateway client script. - Android Configuration Updates: The configuration files for Android builds (
.github/config/android-arm.jsonand.github/config/android-arm64.json) are updated. Both now specify thetest_e2e_device_typeas 'android'. Theandroid-arm64config also enablestest_e2eand adds an initial list ofe2e_test_targets, includingbrowse:infra_smoke_test.
Changelog
Click here to see the changelog
- .github/actions/e2e_tests/action.yaml
- Added new composite GitHub Action for running Vega E2E tests.
- Defines inputs for test dimensions, device type, and Vega targets.
- Includes steps for installing requirements, generating gRPC files, setting up gcloud, and triggering tests via
on_device_tests_gateway_client.pywith--test_type "vega_test"and--vega_targets.
- .github/actions/on_device_tests/action.yaml
- Added
test_e2e_device_typeas a required input (line 16-18). - Passed the
test_e2e_device_typeinput as the--device_typeargument to theon_device_tests_gateway_client.pyscript (line 67).
- Added
- .github/config/android-arm.json
- Added
test_e2e_device_typeset to "android" (line 7).
- Added
- .github/config/android-arm64.json
- Added
test_e2e_device_typeset to "android" (line 7). - Added
test_e2eset totrue(line 8). - Added
e2e_test_targetsarray including "browse:infra_smoke_test" (line 18-20).
- Added
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
New tests arrive, a Vega star,
Through actions run, both near and far.
Configs updated, green lights gleam,
Automated, a coder's dream.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces support for Vega E2E tests by adding a new GitHub Action workflow (.github/actions/e2e_tests/action.yaml) and updating an existing one (.github/actions/on_device_tests/action.yaml) along with related configuration files. The changes are well-structured and seem to effectively integrate the new testing capabilities. I have one suggestion for improving clarity in the new action's inputs.
Summary of Findings
- Input Description Clarity: In
.github/actions/e2e_tests/action.yaml, the inpute2e_test_targetshas an inaccurate description that appears to be copied from another input. This should be corrected to reflect its actual purpose.
Merge Readiness
The pull request is largely in good shape and introduces valuable new E2E testing capabilities. However, there is a medium-severity issue regarding the clarity of an input description in the new GitHub Action. I recommend addressing this to improve the maintainability and usability of the action. As I am an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members before merging. Addressing the identified issue would be beneficial prior to merging.
|
gemini /review |
|
Acknowledged the |
|
gemini /review |
|
Acknowledged the |
|
gemini /review |
|
Acknowledged the |
|
gemini can you review my last change? |
|
It looks like you might be trying to trigger another code review. To do that, please use the command If you were asking about a specific change or had a question about the previous review comments, please clarify, and I'll do my best to assist! |
|
gemini review a12de0a |
|
It looks like you're trying to trigger a review for a specific commit ( To trigger a full review of the latest changes in the pull request, please use the command If you have questions about the existing review comments or the overall changes in the pull request, feel free to ask, and I'll do my best to help! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new GitHub Action for E2E tests and updates related configurations, presumably to add support for a "Vega" platform or device. The overall structure is good, and the new action provides a solid foundation for E2E testing.
I've identified a few areas where explicitness, consistency, and flexibility could be enhanced. Addressing these would make the new testing infrastructure more robust and maintainable.
Summary of Findings
- Action Input Explicitness: The
e2e_testsaction relies on an implicitTEST_ATTEMPTSenvironment variable. Making this an explicit input (e.g.,inputs.e2e_test_attempts) would improve the action's clarity and maintainability. - Configuration Key Consistency: The key
test_e2e_device_typein.github/config/android-arm.jsonis inconsistent withtest_device_familyused in other configuration files and action inputs. Aligning these would improve consistency. - Action Flexibility: Artifact Name: The
e2e_testsaction hardcodes the artifact nameCobalt.apkin the GCS path. Parameterizing this (e.g., via aninputs.artifact_name) would make the action more flexible for testing different artifacts. - Action Flexibility: Test Label: The
e2e_testsaction uses a hardcoded labelchrobalt_vega_test. If the action is intended for broader use beyond Vega, parameterizing this label could enhance its reusability.
Merge Readiness
The pull request makes good progress in adding E2E testing capabilities. However, there are a few medium-severity suggestions related to explicitness of action inputs, consistency in configuration, and flexibility of the new E2E action. I recommend addressing these points to enhance the maintainability and reusability of the code. Therefore, I suggest changes before merging. As a reviewer, I am not authorized to approve the pull request; please ensure it undergoes further review and approval as per your team's process.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces support for running Vega E2E tests within the CI workflow by adding a new GitHub Action and updating existing configuration files. The changes seem well-structured and address the requirement to integrate E2E tests. The update to the on_device_tests action to correctly pass the test_attempts input is a good fix.
Summary of Findings
- Input validation for
e2e_test_targets: Thejqcommand in the new E2E action assumes the input is a valid JSON array. Adding validation or error handling for this input could make the action more robust. - Corrected
test_attemptsinput usage: Theon_device_testsaction was updated to correctly use the action input fortest_attemptsinstead of an undefined environment variable, fixing a potential bug.
Merge Readiness
The pull request introduces necessary changes for integrating E2E tests and includes a valuable fix for the on_device_tests action regarding the test_attempts input. There is a medium severity concern about input validation in the new E2E action. I recommend addressing the input validation point before merging. Please have other reviewers approve this code before merging, as I am unable to approve pull requests directly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Libzu
added
on_device
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds Vega support, primarily through a new E2E test GitHub Action and significant updates to the on_device_tests_gateway_client.py to differentiate between unit_test and e2e_test types. The changes look good overall, but there are a few items to address, mainly concerning the handling of device dimensions and family for E2E tests, and improving script robustness.
| if args.test_type == 'unit_test': | ||
|
|
There was a problem hiding this comment.
For unit_test type, filter_json_dir and gcs_archive_path are essential. While the calling GitHub Action provides them, the script itself doesn't enforce their presence if called directly. Adding a runtime check here would make the script more robust and its requirements clearer when test_type is 'unit_test'.
if not args.filter_json_dir:
raise ValueError("--filter_json_dir is required for unit_test type")
if not args.gcs_archive_path:
raise ValueError("--gcs_archive_path is required for unit_test type")There was a problem hiding this comment.
@Libzu , please take a look
There was a problem hiding this comment.
Make required with argparse.
There was a problem hiding this comment.
This is not possible using argument groups. This requires to create subparser for each 'test_type' under subparser 'trigger', but this has other limitations. I explored this option (https://paste.googleplex.com/4607506493734912 - https://duckie.corp.google.com/thread/6940164b-5878-4bbd-8a56-663f4aa5a809), but AI suggests to add a separate block to check required arguments, something like:
if not args.cobalt_path:
parser.error('--cobalt_path is required when --test_type is e2e_test')
elif args.test_type == 'unit_test':
if not (args.device_family and args.gcs_archive_path and args.gcs_result_path and args.filter_json_dir):
parser.error('--device_family, --gcs_archive_path, --gcs_result_path, and --filter_json_dir are required when --test_type is unit_test')
let me know if you think this is not a good option
| elif args.test_type == 'e2e_test': | ||
|
|
There was a problem hiding this comment.
Similarly for e2e_test type, cobalt_path is essential. Adding a runtime check for its presence when test_type is 'e2e_test' would improve script robustness and clarity of requirements.
if not args.cobalt_path:
raise ValueError("--cobalt_path is required for e2e_test type")There was a problem hiding this comment.
@Libzu , please take a look
There was a problem hiding this comment.
Make it required with argparse below instead.
| elif args.test_type == 'e2e_test': | ||
|
|
There was a problem hiding this comment.
Make it required with argparse below instead.
| if args.test_type == 'unit_test': | ||
|
|
There was a problem hiding this comment.
Make required with argparse.
| } | ||
| _DEPS_ARCH_MAP = { | ||
| 'android': '/sdcard/chromium_tests_root/deps.tar.gz', | ||
| 'ssh': '/home/pi/test/', |
There was a problem hiding this comment.
Nit: I think this should include the filename to be consistent with android above.
There was a problem hiding this comment.
I preferred this way to be consistent with the running instructions.
For raspi, the push_files param
push_files=test_runtime_deps:/home/pi/test/
For android, it is:
push_files=test_runtime_deps:/sdcard/chromium_tests_root/deps.tar.gz
In case I have to change this to a filename for ssh, which one would it be?
| trigger_args.add_argument( | ||
| '--device_family', | ||
| type=str, | ||
| default='android', |
There was a problem hiding this comment.
Remove the default and make the flag required instead.
There was a problem hiding this comment.
Removed the default valut.
To set it as required -> Same issue mentioned before. Best option is to add separate block to check arguments...
SortaCookie
force-pushed
the
main
branch
3 times, most recently
from
5de9067 to
0c39678
Compare
copybara-service
bot
force-pushed
the
main
branch
from
0c39678 to
1ce619e
Compare
SortaCookie
force-pushed
the
main
branch
2 times, most recently
from
e5fea95 to
7dd4094
Compare
copybara-service
bot
force-pushed
the
main
branch
from
027f72a to
9c4102d
Compare
SortaCookie
force-pushed
the
main
branch
4 times, most recently
from
36bd2a6 to
230cead
Compare
copybara-service
bot
force-pushed
the
main
branch
from
230cead to
9401684
Compare
SortaCookie
force-pushed
the
main
branch
3 times, most recently
from
6e38faf to
2e3ef2f
Compare
copybara-service
bot
force-pushed
the
main
branch
from
2e3ef2f to
ae74e35
Compare
copybara-service
bot
force-pushed
the
main
branch
from
f3aae0e to
2c8b212
Compare
SortaCookie
force-pushed
the
main
branch
5 times, most recently
from
13162cc to
9ad8257
Compare
copybara-service
bot
force-pushed
the
main
branch
from
290f9ca to
f25e09f
Compare
copybara-service
bot
force-pushed
the
main
branch
from
1993a2c to
f4e01b0
Compare
SortaCookie
force-pushed
the
main
branch
2 times, most recently
from
5fcc30a to
0ca8ad7
Compare
copybara-service
bot
force-pushed
the
main
branch
from
0ca8ad7 to
8aa848c
Compare
copybara-service
bot
force-pushed
the
main
branch
from
0ca8ad7 to
bf52231
Compare
b/416339892