Feature Request: Overwrite coverage report comment when a test fails

[blordpluto]

Problem

When a commit causes a previously passing PR to fail a test, the old coverage comment remains. Since the default philosophy of the action seems to be to maintain and revise the coverage comment with the results of the latest run, it seems like it would make more sense to overwrite the comment with a message, something like:

Coverage report not available due to failing test(s).

Not a big deal, but I think this would be a better developer experience than leaving the stale comment.

[ArtiomTr]

Hello @blordpluto 👋,

Could you please attach your action.yml file and screenshots of comments and action's console? Or a link to a PR? Looks like a bug to me, action should overwrite the report comment by default, if tests fail.

[dalevfenton]

hello @ArtiomTr , not sure if I'm seeing exactly the same as described above, but I'm getting a new set of comments every time new commits are pushed to my PR. I've copied the job from our actions.yml as well as screenshots of the last 4 pushes that have each ended up with comments. Our actions run on push if the event matters.

The repo is private so I can't share it, but if you need more information please let me know.

This is the job in my actions.yml, we have a lerna monorepo and I have a matrix set up to run the tests in parallel in each package, it runs the tests and provides one comment for each package:

run_tests:
    name: Run Jest Tests
    runs-on: ubuntu-18.04
    strategy:
      fail-fast: false
      matrix:
        package-name: [package1, package2, package3]
    steps:
      - name: Checkout Repo
        uses: actions/checkout@v2
        with:
          fetch-depth: 0
      - name: Setup Dependencies. // this just sets up node, yarn, installs packages and caches node_modules
        uses: ./.github/actions/setup
      - name: Run Tests and Comment
        uses: ArtiomTr/jest-coverage-report-action@v2
        with:
          working-directory: packages/${{ matrix.package-name }}
          test-script: yarn run test:ci
          package-manager: yarn
          skip-step: install
          annotations: failed-tests

below are screenshots from my last 4 pushes for one of the packages, (but each package is generating its own comment which is causing the PR to get really spammy):

[dalevfenton]

after looking at the source code I can see that since we are using the push event the action is not attempting to find the existing comments since it doesn't have access to the PR number to look them up with

would it be possible to allow passing a pr-number optional input that I could pass in after grabbing it from something like gh-find-current-pr? Then if that is populated for a non isInPR type it could still try to look up the comment and update instead of posting a new one in generateCommitReport

edit proposed a solution here: #293

[ArtiomTr]

Hello @dalevfenton 👋,

This action works with two event types: pull_request and push. pull_request is a standard way of handling things - this event is emitted each time when PR is opened or receives a new commit. push event is useful, when you don't have a PR and commit directly into the branch (hotfix for example). In that case, the comment is attached to the commit.

Could you please clarify, why you're using a push event when you have a PR? I don't have deep knowledge of GitHub APIs, but I have always been using a pull_request event, and everything was working as intended, without additional configurations.

[dalevfenton]

Hi @ArtiomTr, I don't remember the details exactly but there are some differences about how and when a push event is triggered and what commit gets checked out compared to the pull_request event and those differences matter with the other jobs that are part of our workflow. We could create a completely different workflow that would trigger on pull_request but it's nice to have all the CI in one.

I've tested the code in PR #293 with our workflow and it is correctly able to associate with the pull request if passed the correct PR number as an input, so at least for our use-case it would be a very nice addition. I think I have tests passing so it would just need to have documentation updated at this point.