Test gha gateway#858
Conversation
Test Commit to trigger Kokoro
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Code Review
This pull request updates the test target configurations for Android, removing JUnit tests and changing the test type of cobalt_browsertests to gtest. It also updates the on-device tests gateway client to support browser tests and explicit gtest filters. The review feedback highlights critical issues with target-specific arguments accumulating across loop iterations due to shared list references. Additionally, the reviewer advises against hardcoding target-specific filters and test-type overrides in the Python client, recommending more maintainable, configuration-driven approaches instead.
| if test_type in ('unit_test', 'browser_test'): | ||
| if not device_type or not device_pool: | ||
| raise ValueError('Dimensions not specified: device_type, device_pool') |
There was a problem hiding this comment.
The test_args list is mutated inside the loop (via test_args.extend(...)) without being copied or re-initialized per target. This causes target-specific arguments (like test_attempts) to accumulate across iterations and results in all targets sharing the same list reference.\n\nCopying test_args at the start of the block ensures each target gets its own independent list of arguments.
if test_type in ('unit_test', 'browser_test'):\n test_args = list(test_args)\n if not device_type or not device_pool:\n raise ValueError('Dimensions not specified: device_type, device_pool')| elif test_type in ('e2e_test', 'yts_test', 'yts_wpt_test'): | ||
| test_target = target_data['target'] | ||
| test_attempts = target_data.get('test_attempts', '') |
There was a problem hiding this comment.
Similarly to the unit_test/browser_test block, test_args should be copied here to prevent target-specific arguments from accumulating across iterations and to avoid sharing the same list reference across all targets.
elif test_type in ('e2e_test', 'yts_test', 'yts_wpt_test'):\n test_args = list(test_args)\n test_target = target_data['target']\n test_attempts = target_data.get('test_attempts', '')| if target_name == 'cobalt_browsertests': | ||
| return 'ContentMainRunnerImplBrowserTest.StartupSequence' |
There was a problem hiding this comment.
Hardcoding the gtest filter for cobalt_browsertests directly in the Python client code bypasses the filter JSON file mechanism and reduces flexibility.\n\nInstead of hardcoding this in Python, consider defining this filter in a dedicated filter JSON file (e.g., cobalt_browsertests_filter.json under the filter directory) so that it can be managed consistently with other test targets.
| if 'cobalt_browsertests' in target_name: | ||
| test_type = 'browser_test' |
There was a problem hiding this comment.
Hardcoding a target-name-based check ('cobalt_browsertests' in target_name) to override the test_type to 'browser_test' is a maintainability concern.\n\nThis hack is introduced because the test type for cobalt_browsertests was changed from "browsertest" to "gtest" in test_targets.json. If we instead keep a distinct test type (e.g., "browser_test") in the JSON configuration and map it appropriately in the runner, we can avoid hardcoding specific target names in the Python client.
…tdout and stderr to test_results.txt inside MH_GEN_FILE_DIR so that Mobile Harness client plugin can upload the log file (cobalt_browsertests_log.txt) to GCS.
…DIR so Mobile Harness GCS uploader plugin can upload test log
Verification: https://github.com/youtube/cobalt_sandbox/actions/runs/28919810566/job/85805412561?pr=858
Bug: 483488213