-
Notifications
You must be signed in to change notification settings - Fork 5k
test: drop shard index from bot name #38636
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
Test results for "MCP"2727 passed, 116 skipped Merge workflow run. |
Test results for "tests 1"1 flaky34397 passed, 689 skipped Merge workflow run. |
| node-version: ${{matrix.node-version}} | ||
| command: npm run ttest -- --shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }} | ||
| bot-name: "${{ matrix.os }}-node${{ matrix.node-version }}-${{ matrix.shardIndex }}" | ||
| bot-name: "${{ matrix.os }}-node${{ matrix.node-version }}" |
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.
wouldn't it break the merge via adding all the shard zips into the archive with the same name?
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.
report.zip includes shard index by default, so downloaded reports should be all distinct.
| with: | ||
| report_dir: blob-report | ||
| job_name: ${{ inputs.bot-name }} | ||
| job_name: ${{ inputs.bot-name }}-${{ inputs.shard-index }} |
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.
I think we can even use some random guid or timestamp here, since we do not really care about the artifact name.
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.
getting a random value / timestamp would require one more "script" step here since there's no randomness in github's templating language. i actually like shard-index, I had a couple of instances in the past where I wanted to download a single specific blob to do some experiments.
| node-version: ${{matrix.node-version}} | ||
| command: npm run ttest -- --shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }} | ||
| bot-name: "${{ matrix.os }}-node${{ matrix.node-version }}-${{ matrix.shardIndex }}" | ||
| bot-name: "${{ matrix.os }}-node${{ matrix.node-version }}" |
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.
report.zip includes shard index by default, so downloaded reports should be all distinct.
If we want to compare shard performance grouped by bot name, we can't have the shard index in the bot name.