Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Conversation

fo-code
Copy link
Collaborator

@fo-code fo-code commented Feb 16, 2023

I added Modified Files Coverage support and a new Coverage Checks Publisher including tests for it.
Also, I renamed "Change Coverage" to "Modified Lines Coverage".

Example layout for the Checks Overview:
grafik

@fo-code fo-code requested a review from uhafner February 16, 2023 20:28
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

I will continue the review tomorrow with the checks publisher and the tests...

* value to ensure that the coverage of pull requests is better than the whole project coverage.
*/
MODIFIED_FILES_DELTA(Messages._Baseline_MODIFIED_FILES_DELTA(), "fileCoverage", CoverageChangeTendency::getDisplayColorsForTendency),
MODIFIED_FILES_DELTA(Messages._Baseline_MODIFIED_FILES_DELTA(), "modifiedFilesCoverage", CoverageChangeTendency::getDisplayColorsForTendency),
Copy link
Member

@uhafner uhafner Feb 28, 2023

Choose a reason for hiding this comment

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

While looking at the results I think I made a mistake here. When I am developing a PR normally I am interested in the coverage of my changes.

That means we need the totals (this is already working correctly):

  • coverage overall project
  • changed files
  • changed lines

But when looking at the delta we need:

  • delta overall project - delta overall reference (typically not interesting for large projects)
  • delta changed files - delta changed files reference (how is the coverage of the changed files affected)? Currently we check how it changes with respect to the overall coverage.
  • delta changed lines: here I am not sure whether it makes more sense to compare vs. the whole project or the changed files. What do you think? Or do we need both?

Bildschirm­foto 2023-02-28 um 12 00 31

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that makes sense.
Regarding the delta changed lines: I guess we should compare only against the modified files coverage. The whole project has modules with higher and with lower coverage. When we compare the changes of a module with lower coverage against the whole project, there will be a negative trend, even if the modified files are covered quiet good compared to the rest of their module. Therefore, comparing against the changed files would keep the context hence it more accurate, in my opinion.

@uhafner
Copy link
Member

uhafner commented Feb 28, 2023

@uhafner
Copy link
Member

uhafner commented Feb 28, 2023

Maybe we should also exclude things totally that are not available (as done in the summary). Otherwise it looks a little bit weird with all the n/a:

Bildschirm­foto 2023-02-28 um 13 14 22

@uhafner
Copy link
Member

uhafner commented Feb 28, 2023

While trying to fix the display of the changes table I am wondering what information we should show actually?

There are several ways:

  • The files table shows all files, the changed files table shows only changed files while displaying the whole source code, the changed lines table shows also all changed files while displaying the changed source code only.
  • The files table shows all files and has a toggle (or filter by column) that shows only modified files. The changed lines table shows all changed files while displaying the changed source code only.
  • Or any other options that make sense?

Possible UI control:
Bildschirm­foto 2023-02-28 um 14 08 05

# Conflicts:
#	plugin/src/main/java/io/jenkins/plugins/coverage/metrics/steps/CoverageReporter.java
#	plugin/src/main/java/io/jenkins/plugins/coverage/metrics/steps/CoverageViewModel.java
#	plugin/src/test/java/io/jenkins/plugins/coverage/metrics/steps/GitForensicsITest.java
@uhafner uhafner added the enhancement Enhancement of existing functionality label Mar 1, 2023
@uhafner uhafner changed the title Modified Files Coverage and Checks Publisher Add Modified Files Coverage and Checks Publisher Mar 1, 2023
@fo-code
Copy link
Collaborator Author

fo-code commented Mar 1, 2023

Maybe we should also exclude things totally that are not available (as done in the summary). Otherwise it looks a little bit weird with all the n/a:

Bildschirm­foto 2023-02-28 um 13 14 22

Yes, we should do that. n/a only distracts from what is important.

While trying to fix the display of the changes table I am wondering what information we should show actually?

There are several ways:

* The files table shows all files, the changed files table shows only changed files while displaying the whole source code, the changed lines table shows also all changed files while displaying the changed source code only.

* The files table shows all files and has a toggle (or filter by column) that shows only modified files. The changed lines table shows all changed files while displaying the changed source code only.

* Or any other options that make sense?

Possible UI control: Bildschirm­foto 2023-02-28 um 14 08 05

I suggest the second option - that sounds good. By default, we should show only the changed files in my opinion, since most of the time, that's what is interesting - like it is in the screenshot.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

I think the PR is now ready to be integrated. I'll track the remaining tasks in #512.

@uhafner uhafner merged commit 9a9232a into jenkinsci:coerage-model Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Enhancement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants