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

Reinstate CI unit test coverage #2274

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Reinstate CI unit test coverage #2274

merged 4 commits into from
Mar 15, 2023

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Mar 14, 2023

Launch Checklist

PR #2044 removed the unit test coverage reports. This PR reinstates them and uploads the coverage reports as build artifacts.

Per #2044 (comment)

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@rotu rotu changed the title Add code coverage to unit test output Reinstate CI unit test coverage Mar 14, 2023
@rotu rotu marked this pull request as ready for review March 14, 2023 19:22
@rotu rotu marked this pull request as draft March 14, 2023 19:24
text reporter provides output in ci log
html-spa reporter provides friendly and interactive output
@rotu
Copy link
Contributor Author

rotu commented Mar 14, 2023

@HarelM Coverage reports are a cool resource, but we need to increase the baseline coverage (maybe include integration tests too?) before this report really becomes useful.

https://github.com/maplibre/maplibre-gl-js/suites/11560050540/artifacts/598609701

@rotu rotu marked this pull request as ready for review March 14, 2023 20:13
@HarelM
Copy link
Collaborator

HarelM commented Mar 14, 2023

I agree coverage should be increased, but I want to be able to easily see the current coverage on a file that was changed as part of a PR and thus let the PR owner know that they need to improve coverage.
Currently, this is far from easy, even with the zip you added, it's still too complicated.
I tried to add a PR comment about coverage but it didn't work well.
I guess codecov is the best solution for this, but it needs some setup.
In any case, base console coverage is better than nothing.

Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

Does this need .gitignore changes?

@rotu
Copy link
Contributor Author

rotu commented Mar 14, 2023

Yeah, this is not geared toward

I want to be able to easily see the current coverage on a file that was changed as part of a PR and thus let the PR owner know that they need to improve coverage. Currently, this is far from easy, even with the zip you added, it's still too complicated.

Can you point me to a repo that does this well? I bet it's not too hard to check for coverage changes on PR (though I still have reservations about doing this while integration/render tests aren't considered in coverage numbers).

I tried to add a PR comment about coverage but it didn't work well.

I don't know what you mean by this.

Does this need .gitignore changes?

Nope. coverage is already in the gitignore.

@HarelM
Copy link
Collaborator

HarelM commented Mar 14, 2023

See here - I was hoping this action might do the trick, there might be other actions, IDK, I'm using codecov in another project and it works well.
ArtiomTr/jest-coverage-report-action#313 (comment)

This is my other project:
https://app.codecov.io/gh/IsraelHikingMap/Site

@rotu
Copy link
Contributor Author

rotu commented Mar 15, 2023

See here - I was hoping this action might do the trick, there might be other actions, IDK, I'm using codecov in another project and it works well. ArtiomTr/jest-coverage-report-action#313 (comment)

This is my other project: https://app.codecov.io/gh/IsraelHikingMap/Site

I'm keeping this PR as-is. Let's see what we can do with the current coverage reports before we add another action item to each PR.

@HarelM HarelM merged commit d74a5bc into main Mar 15, 2023
@HarelM HarelM deleted the rotu-patch-1 branch March 15, 2023 15:28
manhcuongincusar1 pushed a commit to manhcuongincusar1/maplibre-gl-js that referenced this pull request Mar 16, 2023
* Unit test coverage

* rename upload step

* use my favorite test reporters

text reporter provides output in ci log
html-spa reporter provides friendly and interactive output

* upload coverage even if unit tests fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants