-
Notifications
You must be signed in to change notification settings - Fork 187
Add Browser Tests and YTS WPT Tests to Presubmit #9052
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request integrates Cobalt Browser tests and YTS WPT tests into the GitHub CI workflow by updating the configuration and the test gateway client script. The changes look good overall, but I've identified two potential issues. First, the configuration enables yts_playback tests, but the client script doesn't handle this test type, which will cause an error. Second, the handling for the new browser_test and yts_wpt_test types in the Python script is incomplete and duplicates code, which will likely cause those tests to fail. I've left detailed comments with suggestions on how to address these points by adding support for the new test type and refactoring the duplicated logic.
| elif test_type in ('browser_test', 'yts_wpt_test'): | ||
| test_type = 'e2e_test' | ||
| test_target = target_data['target'] | ||
| test_attempts = target_data.get('test_attempts', '') | ||
| if test_attempts: | ||
| test_args.extend([f'test_attempts={test_attempts}']) | ||
| elif args.test_attempts: | ||
| test_args.extend([f'test_attempts={args.test_attempts}']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new block for browser_test and yts_wpt_test is missing logic that is present in the e2e_test/yts_test handler. Specifically, it's missing the addition of test_target to test_args, and appending test_root_target, system_image_download_url, and cobalt_path to files. This will likely cause these new tests to fail.
Instead of duplicating the logic, I recommend refactoring to combine this with the preceding elif block. You can modify the condition at line 248 to include the new test types and then add a nested if to handle the test_type reassignment. This will fix the bug and improve maintainability by removing code duplication.
Here's a suggested implementation:
- Change line 248 to:
elif test_type in ('e2e_test', 'yts_test', 'browser_test', 'yts_wpt_test'): - Add the following lines after line 248:
if test_type in ('browser_test', 'yts_wpt_test'): test_type = 'e2e_test'
- Remove this
elifblock (lines 264-271).
|
Don't we want this on arm64 targets as well? |
.github/workflows/main.yaml
Outdated
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: ci-essentials-${{ inputs.platform }} | ||
| - name: Run YTS Tests |
Check warning
Code scanning / Scorecard
Pinned-Dependencies Medium
Remediation tip: update your workflow using https://app.stepsecurity.io
Click Remediation section below for further remediation help
| ) | ||
| runs-on: [self-hosted, odt-runner] | ||
| name: ${{ matrix.name }}_yts_wpt_tests | ||
| permissions: {} |
Check warning
Code scanning / Scorecard
Pinned-Dependencies Medium
Remediation tip: update your workflow using https://app.stepsecurity.io
Click Remediation section below for further remediation help
Integrate Cobalt Browser tests and YTS WPT tests into the GitHub CI workflow.
Bug: 473909877