[typespec-ts] Add Spector coverage report workflow - #5312
[typespec-ts] Add Spector coverage report workflow#5312Jialin Huang (JialinHuang803) wants to merge 26 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7d904e02-8f6f-49d8-93dc-11fc6a458486
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the companion skill from PR Azure#5282 instead of duplicating it in this branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No changes needing a change description found. |
|
You can try these changes here
|
Avoid creating or updating a task while Copilot or an implementation PR is active. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| - uses: ./.github/actions/setup | ||
|
|
||
| - name: Install dependencies | ||
| run: pnpm install |
There was a problem hiding this comment.
| run: pnpm install | |
| run: pnpm install --filter "@azure-tools/typespec-ts..." |
you can do that too for slight speed bonus
There was a problem hiding this comment.
Seems @typespec/monorepo is required, so "@azure-tools/typespec-ts..." --filter "@typespec/monorepo" is added
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| const fs = require("fs"); |
There was a problem hiding this comment.
can this script be moved out of this into a ts file (and use modern js not common js)
There was a problem hiding this comment.
Moved the script to .github/scripts in 0dab7f7.
| const validStatuses = new Set(["pass", "fail", "not-implemented"]); | ||
| const notImplementedByPackage = new Map(); | ||
| const summaries = []; | ||
| const scenarioSources = new Map([ |
There was a problem hiding this comment.
does the report job also need to check github api? feels a little of duplication/no good split between the job here
There was a problem hiding this comment.
The report job needs the GitHub api to locate the existing report and update the content. The task job uses the api for task detection and deduplication.
There is also a 'report-only' mode which can run only the report job when triggering the workflow manually.
There was a problem hiding this comment.
can't it just find the issue in the next step and replace the content with the latest value, this all feels very complex here to reparse all the markdown expecting it to stay in the same format
There was a problem hiding this comment.
The idea of reparsing the existing markdown is to preserve manually edited Skip Implement checkboxes and Comments in the report and identify new scenarios. The report job doesn't fully replace the report content.
There was a problem hiding this comment.
I mean is that really going to be reliable, this feels like if the comment is edited in the wrong way the parsing will fail and erase everything anyway.
There is explicit config you can have with spector to mark a test as unsupported we probably should rely on that instead no?
There was a problem hiding this comment.
Yes, that makes sense. Let me investigate if we can leverage the spector.config.yaml to make things easier.
Regarding the config to mark a test as unsupported, do you mean setting the scenario group to false as below?
# Disabled: multipart not yet supported
payload/multipart: false
I am thinking about marking the scenario group as skipped in the report if the scenario group is set to false in the config. However, this may introduce other problems. Seems we couldn't set a specific scenario to false in spector.config. If we want to implement some scenarios while leaving other scenarios in the same scenario group not-implemented, the scenario group in the config should still be set to true.
And the reason why I added a Comment column to the report is that we can quickly scan or edit why we need to skip the tests. We had been using the report workflow for a while in the previous autorest.typescript repo, and most comments we added are just some texts or links which are less likely to cause the parsing to fail. Maybe we can try this and see what problems we can have then fix it.
| working-directory: packages/typespec-ts | ||
| run: pnpm copy:typespec | ||
|
|
||
| - name: Run Spector tests |
There was a problem hiding this comment.
Do you actually need to run the test, can you not just simply check the config and compare with the tests from spector?
There was a problem hiding this comment.
The tests are run because the coverage JSON contains the most correct and direct status of the spector tests, while it's more complicated to check the spec config and search for tests that are skipped or partially implemented to get the final implementation results.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds a scheduled Spector coverage workflow for the TypeScript emitter. The workflow:
@azure-tools/typespec-ts, runs its Spector suite, and readsspector-coverage-typescript-azure.json.JialinHuang803andkazrael2119.Workflow outputs
The workflow writes its results to GitHub issues; it does not commit generated files or publish a build artifact.
JialinHuang803andkazrael2119. Manual runs can suppress this output withreport_only. See the task produced in the fork.A task is considered active when it is assigned to Copilot or linked to an open implementation PR. In that case, the workflow leaves all existing task issues unchanged and creates no replacement. This behavior was exercised in the fork with active task #4 and implementation PR #7.
The implementation task uses the
typespec-ts-add-spector-testskill from #5282.