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

Option to only show coverage for files where coverage changed #18

Open
Weetbix opened this issue Dec 17, 2020 · 7 comments
Open

Option to only show coverage for files where coverage changed #18

Weetbix opened this issue Dec 17, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@Weetbix
Copy link

Weetbix commented Dec 17, 2020

Currently the action shows coverage for all files in the project.

It would be great if we could show:

  • The total coverage for the monorepo project
  • Coverage per file only for files where the coverage changed

This would allow the action to be more usable in a repository with a large amount of files.

@Weetbix Weetbix added the enhancement New feature or request label Dec 17, 2020
@janory
Copy link

janory commented Feb 4, 2021

This would be very cool, because the action currently can't deal with the amount of the files in one of our repos.
GitHub rejects the API call to create a comment, because the content is too big.

Listing only the changed files would be a good solution, @marinakalyuzhnaya alternative idea was to remove the expandable part and upload the generated html file to GH artifacts and provide a link to that in the comment. Also a nice solution.

@Weetbix Weetbix changed the title Option to only show coverage for changed files Option to only show coverage for files where coverage changed Feb 5, 2021
@Weetbix
Copy link
Author

Weetbix commented Feb 5, 2021

@janory Yeah I think there are a couple of approaches.

I updated the issue to be more specific, that the coverage should only be shown for files where the coverage has changed. Only checking file changes will not be good because deleting tests for a module without modifying the module, would not list the module in the output even though its test coverage has changed.

For the simple solutions without diffing, I think we can either upload the results as HTML (IF clicking the link really renders the HTML in the browser) or we can even just stop showing line analysis when there are more than 20 or so files. At the the point where there are hunders of lines of files in the comment its not useful and no one is looking at it anyway.

@marinakalyuzhnaya
Copy link

I guess the solution will be be to filter only files, where were some changes (updates, delete, new lines) change . And we can get a list of modified files directly in GH https://docs.github.com/en/rest/reference/pulls#list-pull-requests-files

@Weetbix
Copy link
Author

Weetbix commented Feb 5, 2021

@marinakalyuzhnaya that doesn't work well because we can lose coverage on files that are not included in the changed files list, and then that will not be visible. For example if you delete tests: ComponentA.tests.tsx but dont change ComponentA.tsx, the coverage will drop for the component but it wont be reported in the comment.

I think the best approach is to use diff view that we have, then we can include only files that have a changed diff

If we need a short term solution I would just cap the max number of files to be shown, as its not useful to show hunders of lines of files anyway

@janory
Copy link

janory commented Feb 5, 2021

Hmm... 🤔

As far as I see you guys have a couple of ideas to solve this already! 💪
I would like to make this usable for our projects asap and later we can introduce more complex features, would it be okay for you, if I would add two extra input props for the action?

Namely:

  • hide-details, by default this would be false
  • app-name, this would only make sense for single repos, as you know, one of our repos is not a true monorepo and this would be useful to label coverage report.

Are you okay, if I create a PR with these improvements?
I think these two extra props won't pollute the action with custom stuff, because this two things can be useful for everyone else as well. Once we have a more sophisticated logic for hiding the details or show only the changed stuff or just strip after certain number of lines you can kill the hide-details feature.

If you are against these two props in the mater, I will just create a side branch and will use that in our repo. Let me know.

@marinakalyuzhnaya
Copy link

marinakalyuzhnaya commented Feb 5, 2021

Hmm... 🤔

As far as I see you guys have a couple of ideas to solve this already! 💪
I would like to make this usable for our projects asap and later we can introduce more complex features, would it be okay for you, if I would add two extra input props for the action?

Namely:

  • hide-details, by default this would be false
  • app-name, this would only make sense for single repos, as you know, one of our repos is not a true monorepo and this would be useful to label coverage report.

Are you okay, if I create a PR with these improvements?
I think these two extra props won't pollute the action with custom stuff, because this two things can be useful for everyone else as well. Once we have a more sophisticated logic for hiding the details or show only the changed stuff or just strip after certain number of lines you can kill the hide-details feature.

If you are against these two props in the mater, I will just create a side branch and will use that in our repo. Let me know.

I think this will be needed improvement for single repo. Otherwise it cant be used in our repo.

@Weetbix
Copy link
Author

Weetbix commented Feb 5, 2021

Yep sounds good, I think hide-details makes sense.

app-name also makes sense for the case where you want to run the action twice in on repository, and that repository is not a mono repo. That will allow you to produce two or more unique comments per PR.

@janory janory linked a pull request Feb 5, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants