-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add workflow to comment conformance tests diff #19555
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
Conversation
e4cb4d5 to
3a0a354
Compare
3a0a354 to
d781a6a
Compare
…19556) ## Summary This PR adds a workflow to comment the diff of diagnostics when running ty between `main` and a pull request on the [typing conformance test suite](https://github.com/python/typing/tree/main/conformance/tests). The main workflow is introduced in #19555 which this workflow depends on. This workflow is similar to the [mypy primer comment](https://github.com/astral-sh/ruff/blob/d781a6ab3f44afe5dd1afe1d06fb58fd3739fab5/.github/workflows/mypy_primer_comment.yaml) workflow. ## Test Plan I cannot test this workflow without merging it on `main` unless anyone knows a way to do this.
d781a6a to
e8fee63
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
This reverts commit d0db993.
|
|
The mypy-primer workflow got triggered as I made a change in the Rust code which added the diff, but it did not get triggered once I reverted the Rust code change so the diff has persisted. |
…19556) ## Summary This PR adds a workflow to comment the diff of diagnostics when running ty between `main` and a pull request on the [typing conformance test suite](https://github.com/python/typing/tree/main/conformance/tests). The main workflow is introduced in #19555 which this workflow depends on. This workflow is similar to the [mypy primer comment](https://github.com/astral-sh/ruff/blob/d781a6ab3f44afe5dd1afe1d06fb58fd3739fab5/.github/workflows/mypy_primer_comment.yaml) workflow. ## Test Plan I cannot test this workflow without merging it on `main` unless anyone knows a way to do this.
sharkdp
left a comment
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.
Thank you
| echo "new commit" | ||
| git checkout -b new_commit "${{ github.event.pull_request.head.sha }}" | ||
| git rev-list --format=%s --max-count=1 new_commit | ||
| cargo build --release --bin ty |
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 guess we could experiment with whether or not a release build is faster here (longer build time vs faster check time)
|
|
||
| echo "Running ty on old commit (merge base)" | ||
| cd conformance/tests | ||
| ../../../ruff/ty-old check --color=never --output-format=concise . > ../../../old-output.txt 2>&1 || true |
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.
Elsewhere, we fail the build if the exit code indicates a panic. But I guess it's fine if we see it in the diff?
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.
Yeah, we can experiment, not sure what's the right approach is here.
carljm
left a comment
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.
Looks great, thank you!
Summary
Follow-up to #19556, this PR adds the workflow that computes the diagnostic diff which the workflow introduced in the linked PR will add as a comment.
This workflow is similar to the ty ecosystem-analyzer workflow.
Closes: astral-sh/ty#212
Test Plan
Use the comment history to look at the diff output where the order of the history corresponds to the steps mentioned above in reverse order i.e., the edit in the middle will contain the diff output: