Skip to content
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

Finding also crashes in regression finder #2113

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Mar 29, 2024

  • previously svg2png crashes, timeouts and leaks have been ignored, but with new step now are now being caught - this depends partially on Random crashes/leaks, when trying to convert invalid lottie files to gifs #2072
  • using github actions to create github comments instead curl(which did not work, I do not know why) - thanks to this, a comment will be created for each PR after the first passing of the CI, indicating whether or not changes in test suite have been detected in the PR, and on subsequent passes the same comment will be updated with newer information
  • re-enabling build with address/undefined sanitizer - looks that all strange freeze have been fixed

Not sure why, but comments not working(maybe will work after merging?) due error

 /home/runner/work/_actions/peter-evans/create-or-update-comment/v4/dist/index.js:4695
      const error = new requestError.RequestError(toErrorMessage(data), status, {
                    ^

RequestError [HttpError]: Resource not accessible by integration
    at /home/runner/work/_actions/peter-evans/create-or-update-comment/v4/dist/index.js:4695:21
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  status: 403,

peter-evans/create-or-update-comment#76

this worked fine on different project - https://github.com/qarmin/SVG-regression-finder/actions/runs/8472470087/job/23214648273
and added/updated comment: qarmin/SVG-regression-finder#5 (comment)

Also ruff uses similar workflow - https://github.com/astral-sh/ruff/blob/main/.github/workflows/pr-comment.yaml

https://docs.github.com/en/code-security/code-scanning/troubleshooting-code-scanning/resource-not-accessible

@qarmin qarmin force-pushed the new_regression_mode_comments branch from 20f1caa to de4d032 Compare March 29, 2024 19:12
@qarmin qarmin marked this pull request as ready for review March 29, 2024 19:21
@qarmin qarmin requested a review from hermet as a code owner March 29, 2024 19:21
@qarmin qarmin force-pushed the new_regression_mode_comments branch from de4d032 to 5392892 Compare March 29, 2024 19:31
@hermet hermet added the infrastructure Dev infrastructure label Mar 29, 2024
@hermet hermet requested review from tinyjin and JSUYA March 30, 2024 08:31
@hermet hermet force-pushed the new_regression_mode_comments branch from 5392892 to 6930198 Compare April 3, 2024 04:27
@hermet hermet added the enhancement Improve features label Apr 3, 2024
@hermet
Copy link
Member

hermet commented Apr 4, 2024

@qarmin Thanks for your contribution. We might need to clarify the regression failure here.

@qarmin qarmin force-pushed the new_regression_mode_comments branch from c44d8c1 to 3e0d5aa Compare April 4, 2024 14:28
@qarmin qarmin changed the title Finding crashes and adding comments Finding also crashes in regression finder Apr 4, 2024
@qarmin
Copy link
Contributor Author

qarmin commented Apr 4, 2024

I have no idea how to make comments automatically add to PR, so I just commented them out so the CI is now green and anyone can check what's not working as I have no idea how to fix it(looks like a permissions problem)

Adding comments to a thread is useful when files that didn't convert properly before are changing. In that case, apart from the information in the CI logs, there is no information that the PR has changed anything. In the case where a regression is found, CI fails and this is easily visible.

@tinyjin
Copy link
Member

tinyjin commented Apr 5, 2024

Hello, @qarmin.
Would this link help for the commenting problem?
peter-evans/create-or-update-comment#188

In public repositories this action does not work in pull_request workflows when triggered by forks.

I think it ocurrs because this patch from forked repo. If so, we can just merge and see it works.

@qarmin
Copy link
Contributor Author

qarmin commented Apr 5, 2024

If I understand correctly, after merging, these comments would only be added when a branch is created directly in this repository, e.g. thorvg/branch_fix_10202.
But as far as I can see, no one is doing that, and additionally it would not change anything for outsiders

@tinyjin
Copy link
Member

tinyjin commented Apr 5, 2024

Gotcha, I was misunderstanding. I thought the workflow in PR doesn't work only when Github Action(.yml) changed.
But I see what you're saying is right.

Anyway, this problem only happens in PR which from forked repo due to the token issue.
I've just found this as well, it might solve them.

Alternatively, use the pull_request_target event to comment on pull requests.

The event pull_request_target may unlimit few things on PR instead of pull_request.
I am not sure it would simply work by replacing pull_request to pull_request_target, thinking worth to try.

I see some people do same thing in forked PR like:

name: Thank contributors

on:
  pull_request_target:
    types:
      - closed

jobs:
  thank:
    if: github.event.pull_request.merged == true
    runs-on: ubuntu-latest
    steps:
      - name: Comment on the pull request
        uses: peter-evans/create-or-update-comment@v3
        with:
          issue-number: ${{ github.event.pull_request.number }}
          body: |
            Thank you for contributing! 🎉

Sharing related article :)
https://dev.to/kalkwst/workflow-triggering-events-and-event-actions-5cec#understanding-event-activity-types

@hermet
Copy link
Member

hermet commented Apr 5, 2024

But as far as I can see, no one is doing that, and additionally it would not change anything for outsiders

@qarmin You got a write access. please do it if it's necessary, it's recommended for thorvg maintenance. Thanks.

@hermet hermet merged commit 7a95510 into thorvg:main Apr 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve features infrastructure Dev infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants