Implement code coverage reports on pull requests 🔍

[JamieSlome]

To encourage contribution best practices, we should run coverage checks on critical logic in the library against our current test suite.

Tasks

Beta Give feedback

1. Use nyc to generate test coverage reports
2. Assess coverage reports on new code only (if possible)
3. Implement a GitHub Action to present and block PRs if coverage on new logic is below 80%
4. Rename test files to *.test.js

[JamieSlome]

This Action could be a nice display function for the result of a code coverage report:
https://github.com/marketplace/actions/code-coverage-report

[CyberCitizen01]

Hi @JamieSlome 👋🏻

I'm thrilled to explore the opportunity to contribute to the project as a first-time contributor. I noticed that this issue has been labelled as a "Good First Issue," and I'm eager to take on the challenge.

As a newcomer, I'm committed to learning and adhering to the project's conventions and guidelines. While I've already reviewed the contributing guidelines in the repository, I was hoping to gain further insight into the specific requirements and expectations for this particular issue.

Could you kindly provide any additional information or context that would assist me in tackling this "Good First Issue"? I'm excited to demonstrate my skills and make a positive impact on the project.

In the meantime, I'll be actively preparing and researching to ensure I approach the task with the best possible understanding.

Thank you for your time and consideration. I'm looking forward to collaborating with the team and contributing meaningfully to the project.

[JamieSlome]

@CyberCitizen01, thanks for getting involved!

I'd recommend starting with reading this article as it should give you a good overview of what code coverage is, and why it is important.

Next, I'd take a look through Istanbul's code coverage documentation. This will give you a good idea of how to run code coverage against our codebase, including how to install it.

Let me know if there is anything else I can help you with 👍

[CyberCitizen01]

Thank you @JamieSlome! I'll take a close look at these and get started right away.

Also if you can assign the issue to me, I would be really delighted. Thanks!

[JamieSlome]

@CyberCitizen01 - thanks for your PR! Does your contribution address:

"Assess coverage reports on new code only (if possible)"

It would be nice to show contributors the coverage they are missing rather than the coverage for the entire repository every time. Let me know your feedback and thoughts! 👍

[CyberCitizen01]

@JamieSlome - It's a pleasure to be able to contribute!

Yes I wanted to discuss about that particular task.

• Does the workflow needs to test and assess coverage reports on new code or the workflow will test the whole codebase, but present the coverage report about the modified and newly created files in the PR

  Note

  • The second scenario implies that the threshold limit will be evaluated based on the whole codebase.
  • Second scenario can be implemented by using filter-changed-files: true option of the romeovs/lcov-reporter-action action.
  • In the second scenario, since we're limiting the files which we'll test, there are chances that a test run on the entire codebase will never run and I think it'll impact our confidence in the project.

[JamieSlome]

@CyberCitizen01 - it is a pleasure to work with you!

The workflow should test and assess coverage reports on new code only.

My thinking is that I don't want first-time and existing contributors to feel overwhelmed by a massive coverage report on their PR or feel like they have to address the test coverage for the entire repository. Keeping the coverage requirement discrete and relevant to their contribution only is a better starting place in my opinion.

That said, we do need to bring the test coverage of the repository up, so I wonder if we just need to create a clear and distinct issue for increasing project test coverage to 80%?

General thoughts on the above? 🤔

[CyberCitizen01]

Hi @JamieSlome
Sorry for the late reply got caught up in something else.

I agree with you here that contributors should not be burdened with addressing the test coverage for the entire repository. I'll try to come up with some scripting to assess only the code that a particular contributor is concerned with in his/her PR.

And yes I think we should create a issue for meeting the 80% bar.

[JamieSlome]

@CyberCitizen01 - no problem at all!

Great, that sounds like a plan. I am sure there are existing Actions that already implement this functionality, or at least allow you to control, through the configuration of the GitHub Action, what files and directories you want to review (i.e. only the ones that have changed in the PR).

I have created #240 to increase our test coverage 👍

As always @CyberCitizen01, thank you for your time and contribution! ❤️

[JamieSlome]

@coopernetes @CyberCitizen01 - looking at the failure of #409 it seems like "Assess coverage reports on new code only (if possible)" may not be functioning as expected? Can you confirm whether this is the case?

[coopernetes]

I believe this is a simple GitHub permission problem.

@maoo can you verify if the org has a default "read only" setting for workflows? You may need to reconfigure this repo to allow coverage reports to be posted as comments on PRs.

https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#setting-the-permissions-of-the-github_token-for-your-organization

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#setting-the-permissions-of-the-github_token-for-your-repository

[coopernetes]

cc @finos-admin

[JamieSlome]

@coopernetes @maoo, let's get this solved in our team catchup today if possible. If we are unable, we should probably revert the PR as to be mindful of blocking other PRs 👍

[maoo]

The workflows permissions on this repo (settings) are: Read and write permissions - Workflows have read and write permissions in the repository for all scopes., which I understand is the permissive option.

I read in the docs that - Anyone with write access to a repository can modify the permissions granted to the GITHUB_TOKEN, adding or removing access as required, by editing the permissions key in the workflow file., and I see it in the code - https://github.com/finos/git-proxy/actions/runs/7652616484/workflow?pr=409#L23

However, if you check the build logs, you'll see that the permission is not properly set - https://github.com/finos/git-proxy/actions/runs/7652616484/job/20852745697?pr=409#step:1:18

Docs on https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions are a bit generic, and it may be that we need more permissions; could you try playing a bit with it, for example setting all permissions to write and see if that works?