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

move from SonarCloud to reporting code quality and coverage through GitHub Actions #369

Open
egpbos opened this issue Dec 22, 2023 · 9 comments
Labels
generated-package Related to the generated package, i.e. after running cookiecutter

Comments

@egpbos
Copy link
Member

egpbos commented Dec 22, 2023

GEMDAT uses GitHub Actions to generate a coverage badge, which is published to a GitHub Gist and from there can be included in the README.md. See https://github.com/GEMDAT-repos/GEMDAT/blob/main/.github/workflows/tests.yaml

Something like this could also perhaps be done for code quality.

This way, we could remove SonarCloud as an external dependency, which would simplify setup. Ideally, the action would be published separately, so that not every generated package has to maintain this workflow themselves, but that could be a next step as well.

It would also fix the issue in #221, because creating a badge doesn't have to be done in a PR, and that's the only place where the secret is necessary in the workflow.

@v1kko or @stefsmeets would you be interested in (helping) contributing this?

@egpbos egpbos added the generated-package Related to the generated package, i.e. after running cookiecutter label Dec 22, 2023
@stefsmeets
Copy link
Member

Yeah, I can have a go at this.

@stefsmeets stefsmeets self-assigned this Jan 8, 2024
@stefsmeets stefsmeets mentioned this issue Jan 8, 2024
@stefsmeets
Copy link
Member

Hi @egpbos I added the workflow in the PR. I'm not sure if this fixes #221, because a user will still need to edit the workflow and add their gist ID before the workflow passes.

@stefsmeets stefsmeets removed their assignment Jan 8, 2024
@bouweandela
Copy link
Member

In the guide we recommend using a code coverage service, so we should do the same in the template. A badge is nice for potential users so they can quickly get an impression of the quality of the software, but it lacks the detail required for a healthy development process. Code quality services provide their own badge with overall coverage on it, so I don't really see the added benefit of a custom workflow to compute that.

@egpbos
Copy link
Member Author

egpbos commented Jan 10, 2024

Hrm, good points @bouweandela. Interactively browsing the code annotated with (lack of) coverage is of course a big benefit of those services.

@stefsmeets
Copy link
Member

The same can of course be easily achieved by running this tool locally rather than to rely on a CI to do this for you. This is my preferred way, because it allows me to iterate faster. I rarely looked at sonarcloud when I still used it and found it to be a nuisance.

Obviously, there is no added benefit of using a custom badge as proposed in #370 other than to reduce your dependency on third-party services.

If you think it is better to promote the use of sonarcloud that also works for me. Feel free to close this issue and PR then.

@bouweandela
Copy link
Member

bouweandela commented Jan 11, 2024

I agree that it is more convenient to look at code coverage locally when developing code yourself, but for reviewing pull requests, it is much more convenient to look at CI. A code coverage service will also alert you of lines of code with changed coverage that might be unexpected (for example, this can happen when you refactor and remove some code and associated tests that used code that is shared with other parts of the code, but then is no longer tested). That is why we recommend running the unit tests in CI and using a code coverage service such as Codecov or Coveralls in the Guide.

From my point of view, Sonarcloud can be dropped because it is not based on the ruff linter settings that we use in the template for linting, but rather on Sonarcloud's own linter rules, which provides an inconsistent development experience.

@v1kko
Copy link
Member

v1kko commented Jan 16, 2024

Would it be possible to put the html output of a coverage tool (something like coverage report html) into the documentation? In this way it can be accessed without needing a third party tool.

disclaimer: I have never done something like that, so I am not certain on the amount of work required

@v1kko
Copy link
Member

v1kko commented Jan 16, 2024

Seems options already exist to do it in pull requests:

@egpbos
Copy link
Member Author

egpbos commented Jan 16, 2024

These look very nice, especially Barecheck. They don't offer a full replacement of an interactively browseable site (yet), though. The code review annotations look nice, but also a bit confusing ("24 line is not covered with tests"? Are 24 lines not covered? Or only line 1 in that screenshot? It's not clear to me...), but let's keep watching it, maybe it will get better.

Btw, the feature they promote that "Instead of showing you all uncovered files Barecheck show only the ones you have changed." actually makes it impossible to catch cases that @bouweandela mentioned: "when you refactor and remove some code and associated tests that used code that is shared with other parts of the code, but then is no longer tested". Hopefully that will become optional in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generated-package Related to the generated package, i.e. after running cookiecutter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants