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 Jul 31, 2023

Adds a REST API that provides the coverage information of modified lines for each modified file - see #731 .

@uhafner uhafner added the enhancement Enhancement of existing functionality label Aug 1, 2023
fo-code

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #737 (04dfec4) into master (2ef24aa) will increase coverage by 0.39%.
Report is 15 commits behind head on master.
The diff coverage is 97.87%.

@@             Coverage Diff              @@
##             master     #737      +/-   ##
============================================
+ Coverage     74.08%   74.47%   +0.39%     
- Complexity     1698     1749      +51     
============================================
  Files           130      135       +5     
  Lines          6289     6393     +104     
  Branches        677      690      +13     
============================================
+ Hits           4659     4761     +102     
- Misses         1407     1408       +1     
- Partials        223      224       +1     
Files Coverage Δ
.../plugins/coverage/metrics/restapi/CoverageApi.java 100.00% <100.00%> (ø)
...overage/metrics/restapi/FileWithModifiedLines.java 100.00% <100.00%> (ø)
...ins/coverage/metrics/restapi/LineCoverageType.java 100.00% <100.00%> (ø)
...s/coverage/metrics/restapi/ModifiedLinesBlock.java 100.00% <100.00%> (ø)
...rage/metrics/restapi/ModifiedLinesCoverageApi.java 100.00% <100.00%> (ø)
...metrics/restapi/ModifiedLinesCoverageApiModel.java 83.33% <83.33%> (ø)
...gins/coverage/metrics/steps/CoverageViewModel.java 54.13% <66.66%> (-0.07%) ⬇️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

fo-code

This comment was marked as resolved.

@fo-code fo-code marked this pull request as ready for review August 31, 2023 15:50
modifiedLinesBlocks);
filesWithModifiedLines.add(changedFile);
}
return filesWithModifiedLines;
Copy link
Member

Choose a reason for hiding this comment

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

I have a student implementation of such a code in the coverage-model (not yet published). So this code is ok for now but might be removed in the future...


int currentLine = modifiedLines.get(0);
for (int i = 0; i < modifiedLines.size(); i++) {
if (i == modifiedLines.size() - 1 || !modifiedLines.get(i).equals(modifiedLines.get(i + 1) - 1)) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: this implementation is only partly correct. Since coverage calculation is done on the byte code, there might be lines without coverage information between two covered lines.

Example:

1 int code;
2 // empty
3 code = 0;

Will have a coverage of line 1 and 3 but not 2. As a user I would expect that the block 1-3 is fully covered. But your code produces two blocks. But this can be improved later one if someone complains 🤷

@fo-code
Copy link
Collaborator Author

fo-code commented Oct 7, 2023

@uhafner I extended the GitForensicsITest with a call to the new API to verify the resulting JSON. I added that in that test class since it offers a special setup where modified code lines as well as related coverage data is provided. Both is required for a decent test scenario here.
Also, I finished the implementation and added a description of the new API to the README.

From my side we can merge this PR, if you don't have any further suggestions.

@fo-code fo-code requested a review from uhafner October 7, 2023 10:35
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.

Almost ready for merge, thanks for updating!

/**
* Tests {@link FileWithModifiedLines}.
*/
class FileWithModifiedLinesTest {

Check notice

Code scanning / CodeQL

Unused classes and interfaces

Unused class: FileWithModifiedLinesTest is not referenced within this codebase. If not used as an external API it should be removed.
/**
* Tests {@link ModifiedLinesBlock}.
*/
class ModifiedLinesBlockTest {

Check notice

Code scanning / CodeQL

Unused classes and interfaces

Unused class: ModifiedLinesBlockTest is not referenced within this codebase. If not used as an external API it should be removed.
@fo-code
Copy link
Collaborator Author

fo-code commented Oct 14, 2023

@uhafner I reworked the implementation and included your comments. Now everything works like discussed and is also tested.

The only problem is there are test failures regarding docker images and I think they are not caused by this pull request, so I do not know how to fix it.

@fo-code fo-code requested a review from uhafner October 14, 2023 17:38
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.

Thanks!

@uhafner
Copy link
Member

uhafner commented Oct 17, 2023

I am rerunning the checks and will merge if everything is back to green...

@uhafner uhafner merged commit 582b210 into jenkinsci:master Oct 17, 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.

3 participants