-
Notifications
You must be signed in to change notification settings - Fork 344
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
Infrastructure: Move Regression coverage to PR comment #1517
Conversation
Ah, looks like the security restrictions on PRs isn't letting this preview from something coming from a fork. I don't think this will be an issue once it's in the main branch. You can see result over here though nschonni#19 (comment) |
d48a8b4
to
f7dac4a
Compare
Using the |
@nschonni which PRs will this add a comment to? All? Or, just PRs that change directories where some of the examples have missing tests? Something like this would be cool for spell check. |
From this part of the config pull_request_target:
paths:
- "examples/**"
- "test/**" This job will only trigger if the PR touches files under the |
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.
One small change to filter out landmark example changes -- but this is awesome, thanks so much! :)
pull_request_target: | ||
paths: | ||
- "examples/**" | ||
- "test/**" |
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.
We should filter out examples/landmarks -- "- !examples/landmarks/**"
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.
Good call! Done
repo: context.repo.repo, | ||
comment_id: botComment.id, | ||
body: commentBody | ||
}) |
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.
Does this replace the comment, not append? Because it we don't replace it will get really long! Seems like it from the documentation but just wanted to be sure.
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.
It replaces it if it exists. You can see it in the "edit" dropdown on my test PR in my fork nschonni#19 (comment)
e6a2ea6
to
dda473f
Compare
dda473f
to
c87b47f
Compare
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.
Super great! Can't wait for this :)
c87b47f
to
cd334e2
Compare
ad40a5a
to
ea8b5b4
Compare
@nschonni thank you again ... looking forward to how this will bring the report to the surface. |
Currently this is a job that is allowed to fail on Travis CI. This moves it to an action that will leave a comment on the PR if any of the regression related files are changed. Additional pushes will update the same comment instead of continuing to create new ones on the PR.
I'm set it so it also runs when the CI config file is touched so you'll see it on this PR, but wouldn't on a PR that doesn't touch anything in theexamples
ortest
folder.