Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for YTS tests by introducing a new GitHub composite action and updating the Android ARM configuration. The changes in the configuration file look correct. However, the new GitHub action in .github/actions/yts_tests/action.yaml appears to have some copy-paste errors from an E2E test action. It's incorrectly configured to run E2E tests instead of YTS tests, which would cause it to fail. I've left specific comments with suggestions to fix these issues.
| --token ${GITHUB_TOKEN} \ | ||
| trigger \ | ||
| --test_type ${TEST_TYPE} \ | ||
| --targets '${{ inputs.e2e_test_targets }}' \ |
There was a problem hiding this comment.
The --targets argument is using inputs.e2e_test_targets, but this input is not defined for this action. This will cause the action to fail. It should use inputs.yts_test_targets.
--targets '${{ inputs.yts_test_targets }}' \| GITHUB_RUN_NUMBER: ${{ github.run_number }} | ||
| GITHUB_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} | ||
| GITHUB_WORKFLOW: ${{ github.workflow }} | ||
| TEST_TYPE: e2e_test |
There was a problem hiding this comment.
This action is for YTS tests, but TEST_TYPE is hardcoded to e2e_test. This should be yts_test to correctly categorize and run the tests.
TEST_TYPE: yts_test| description: "Test dimensions JSON string." | ||
| required: true | ||
| yts_test_targets: | ||
| description: "E2E test targets." |
There was a problem hiding this comment.
The description for the yts_test_targets input is 'E2E test targets.', which is misleading for an action that runs YTS tests. It should be updated for clarity.
description: "YTS test targets."| --label author_id-${GITHUB_PR_HEAD_USER_ID:-$GITHUB_COMMIT_AUTHOR_EMAIL} \ | ||
| --dimensions '${{ inputs.test_dimensions }}' \ | ||
| --cobalt_path "${GCS_ARTIFACTS_PATH}" || { | ||
| echo "Finished running tests..." |
There was a problem hiding this comment.
This echo statement inside the failure block is confusing. The same message 'Finished running tests...' is also printed on success (line 84). This can make it harder to quickly identify a failure from the logs. I'd recommend removing this line from the failure block to avoid ambiguity.
| test_device_family=$(cat ${GITHUB_WORKSPACE}/.github/config/${{ inputs.platform }}.json | jq -rc '.test_device_family // empty') | ||
| echo "test_device_family=${test_device_family}" >> $GITHUB_OUTPUT | ||
| - name: Set YTS test targets | ||
| id: set-e2e-test-targets |
There was a problem hiding this comment.
Shouldn't this be set-yts-test-targets ? set-e2e-test-targets is an id in l.180 (no idea how this works tho :)
6e5d325 to
e5f6892
Compare
| config: [devel] | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
Check warning
Code scanning / Scorecard
Pinned-Dependencies Medium
Issue: 454941148
Issue: 454941148
ba05872 to
cf09978
Compare
Issue: 454941148
cf09978 to
c69cb91
Compare
…yts_tests Issue: 454941148
… into feature/yts_tests Issue: 454941148
1289f3a to
9482847
Compare
9482847 to
102fa36
Compare
05ca899 to
9bca71e
Compare
b06b7d4 to
3b23a8d
Compare
3b23a8d to
44ed002
Compare
e2ea908 to
2a76b87
Compare
2a76b87 to
cb019c3
Compare
2dc2a50 to
4268187
Compare
4268187 to
081512e
Compare
332b587 to
a16764e
Compare
Issue: 454941148