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

Add code coverage action #10340

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Add code coverage action #10340

merged 5 commits into from
Jul 11, 2024

Conversation

PyvesB
Copy link
Member

@PyvesB PyvesB commented Jul 8, 2024

It's been exactly three years today since we last submitted a coverage report to Coveralls. This PR introduces a GitHub workflow that runs whenever a commit is pushed to master. To make sure everything was running as intended, I temporarily removed the master branch constraint before opening this PR, here's an example run: https://coveralls.io/builds/68519008

@PyvesB PyvesB added developer-experience Dev tooling, test framework, and CI github_actions [dependabot only] Pull requests that update GitHub Actions labels Jul 8, 2024
Copy link
Contributor

github-actions bot commented Jul 8, 2024

Messages
📖 ✨ Thanks for your contribution to Shields, @PyvesB!

Generated by 🚫 dangerJS against 7d77727

@chris48s
Copy link
Member

chris48s commented Jul 9, 2024

I had a look at this exact problem recently when I was working on
#10320

I think there's a few points I would highlight here.

First one is minor: I think we should run this on a schedule. We don't currently do a full service test run on every commit to master. I think this is the right call. Although we don't merge super frequently, when we do dependabot updates we do often merge 10 or 20 PRs rapidly. A full service test run on every merge is slow, and does get us into the territory where we're possibly hitting rate limits on some of those api keys we use for testing. Previously we were reporting coverage based on a daily test run. I think once a day is about the right cadence for this. If we're going to have one service test run to make the markdown report and one to report coverage, maybe we do them a few hours apart.

Secondly, when I had a go at this, I was running each individual test suite:

  • core
  • package
  • entrypoint
  • integration
  • services

as a seperate step in the workflow. So mine looked like

Yaml Workflow
...
    steps:
      - name: Checkout
        uses: actions/checkout@v4
  - name: Setup
    uses: ./.github/actions/setup
    with:
      node-version: 20

  - name: Migrate DB
    run: npm run migrate up
    env:
      POSTGRES_URL: postgresql://postgres:postgres@localhost:5432/ci_test

  - name: Run core tests
    run: npm run coverage:test:core
    if: always()
    env:
      mocha_reporter: mocha-junit-reporter
      MOCHA_FILE: junit/core/results.xml

  - name: Run package tests
    run: npm run coverage:test:package
    if: always()
    env:
      mocha_reporter: mocha-junit-reporter
      MOCHA_FILE: junit/package/results.xml

  - name: Run entrypoint tests
    run: npm run coverage:test:entrypoint
    if: always()
    env:
      mocha_reporter: mocha-junit-reporter
      MOCHA_FILE: junit/entrypoint/results.xml

  - name: Run integration tests
    run: npm run coverage:test:integration
    if: always()
    env:
      mocha_reporter: mocha-junit-reporter
      MOCHA_FILE: junit/integration/results.xml
      GH_TOKEN: '${{ secrets.GH_PAT }}'
      POSTGRES_URL: postgresql://postgres:postgres@localhost:5432/ci_test

  - name: Run service tests
    run: npm run coverage:test:services
    if: always()
    env:
      RETRY_COUNT: 3
      GH_TOKEN: '${{ secrets.GH_PAT }}'
      LIBRARIESIO_TOKENS: '${{ secrets.SERVICETESTS_LIBRARIESIO_TOKENS }}'
      OBS_USER: '${{ secrets.SERVICETESTS_OBS_USER }}'
      OBS_PASS: '${{ secrets.SERVICETESTS_OBS_PASS }}'
      PEPY_KEY: '${{ secrets.SERVICETESTS_PEPY_KEY }}'
      SL_INSIGHT_USER_UUID: '${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}'
      SL_INSIGHT_API_TOKEN: '${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}'
      TWITCH_CLIENT_ID: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}'
      TWITCH_CLIENT_SECRET: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_SECRET }}'
      WHEELMAP_TOKEN: '${{ secrets.SERVICETESTS_WHEELMAP_TOKEN }}'
      YOUTUBE_API_KEY: '${{ secrets.SERVICETESTS_YOUTUBE_API_KEY }}'
      mocha_reporter: mocha-junit-reporter
      MOCHA_FILE: junit/services/results.xml

...

Here's an example of a run:

https://github.com/badges/shields/actions/runs/9553559835/job/26332639191

You'll notice that as well as the service tests failing (which we kind of expect), the core and package tests were also failing. The reason for that is this TypeError: undefined is not iterable which is caused by bcoe/v8-coverage#2 We're kind of papering over it with continue-on-error: true (which we've put there because we expect the services to fail, but not everything else) but looking at the logs this PR also has the same issue (which I expected). One thing I didn't really fully dig into is whether this means there are some tests that are not being run or not being run correctly, or bits of the coverage being missed. Did you spend any time looking into that? I found that if you do the coverage run on node 22, that fixes this problem, although it is not the same as the platform we deploy on.

The final thing is a question really. It doesn't look like you're doing to explicitly merge the coverage reports, but it does seem to be happening, looking at coveralls. Any idea how this is working?

@PyvesB
Copy link
Member Author

PyvesB commented Jul 9, 2024

I think we should run this on a schedule.

Will update accordingly 👍🏻

Secondly, when I had a go at this, I was running each individual test suite:

I guess one middle-ground would be to separate in two steps, one that runs non-service coverage, and one that runs service coverage, which aligns with the delineation we've done in package.json with coverage:test vs. coverage:test:services.

One thing I didn't really fully dig into is whether this means there are some tests that are not being run or not being run correctly, or bits of the coverage being missed. Did you spend any time looking into that? I found that if you do the coverage run on node 22, that fixes this problem, although it is not the same as the platform we deploy on.

All tests seems to be running as expected, but having generated a report on Node 22 (excluding flaky service tests), coverage percentage is slightly higher (68.92% vs. 68.7%). I'm guessing bits of coverage are being missed unless something else has changed in Node 22. However, I'm tempted to not agonise over this and maybe just switch them to Node 22? 😄

The final thing is a question really. It doesn't look like you're doing to explicitly merge the coverage reports, but it does seem to be happening, looking at coveralls. Any idea how this is working?

Looking at how things are being generated locally, there seems to be some clever merging happening to append new coverage information into a single lcov.info file. The Coveralls GitHub action then kicks it to upload that file. AT least that' my rough understanding!

@chris48s
Copy link
Member

chris48s commented Jul 9, 2024

I guess one middle-ground would be to separate in two steps

I think it would be useful to separate the run where we expect all tests to pass from the ones where we expect some to fail.

I'm tempted to not agonise over this and maybe just switch them to Node 22?

I think I would also be in favour of just measuring test coverage on node 22 as the least-worst solution. You'll need

  node-version: 22
env:
  NPM_CONFIG_ENGINE_STRICT: 'false'

but lets also leave a comment in the workflow yaml explaining why we're deploying on node 20 and measuring coverage on 22.

@calebcartwright
Copy link
Member

I'll just add that part of the discussion here relates to a broader topic of discussion I was alluding to in #10341 (comment)

@PyvesB
Copy link
Member Author

PyvesB commented Jul 10, 2024

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on board with the latest changes 👍

@PyvesB PyvesB added this pull request to the merge queue Jul 11, 2024
Merged via the queue into master with commit 27f964e Jul 11, 2024
38 checks passed
@PyvesB PyvesB deleted the code-coverage-coveralls branch July 11, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI github_actions [dependabot only] Pull requests that update GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants