-
Notifications
You must be signed in to change notification settings - Fork 76
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
Comments
Yeah, I can have a go at this. |
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. |
Hrm, good points @bouweandela. Interactively browsing the code annotated with (lack of) coverage is of course a big benefit of those services. |
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. |
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 |
Would it be possible to put the disclaimer: I have never done something like that, so I am not certain on the amount of work required |
Seems options already exist to do it in pull requests:
|
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. |
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?
The text was updated successfully, but these errors were encountered: