Try to expose duplicate key exception in #467

[uhafner]

For details see https://github.com/jenkinsci/code-coverage-api-plugin/issues/467

[codecov]

Codecov Report

Merging #475 (bed39f6) into master (fd2bd6a) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #475   +/-   ##
=========================================
  Coverage     72.57%   72.57%           
  Complexity     1006     1006           
=========================================
  Files            87       87           
  Lines          3741     3741           
  Branches        435      435           
=========================================
  Hits           2715     2715           
  Misses          878      878           
  Partials        148      148           

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

[fo-code]

@uhafner do you have an idea why this test is successful, even if it throws an error in the Jenkins environment?
I also debugged it and everything seems totally fine to me...
The path of the file Layouts.java, which causes the problem, is not even part of the affected mapping on my machine.

[uhafner]

@uhafner do you have an idea why this test is successful, even if it throws an error in the Jenkins environment?

Which error? Do you mean the PMD warning?

I also debugged it and everything seems totally fine to me...

Maybe I did not set the correct baselines that caused the issue. Unfortunately, the build with the exception trace has been deleted in the meantime.

The path of the file Layouts.java, which causes the problem, is not even part of the affected mapping on my machine.

[fo-code]

Okay I might have expressed it badly.
The test case you created here reflects the case which created the error at issue #467 perfectly, as I can see. Nonetheless, the test case is successful - no exceptions.
I also debugged the test case you created and everything seems good to me. So what I do not understand is, why this test case works, but #467 exists - even if there are the same commits in use.
So I thought maybe you have an idea, why.

Yes the trace would be helpful, you are right...

[uhafner]

Okay I might have expressed it badly. The test case you created here reflects the case which created the error at issue #467 perfectly, as I can see. Nonetheless, the test case is successful - no exceptions.

Ah, ok. I thought you were talking about the checks of this PR...

I also debugged the test case you created and everything seems good to me. So what I do not understand is, why this test case works, but #467 exists - even if there are the same commits in use. So I thought maybe you have an idea, why.

I'm not sure if the other commit 723aac821a5e967dd3894d215b267a38de086b90 is the correct one. I just picked it, because the day of the commit might be correct. But maybe we need to run the tests with some other commits of the master branch (a few older or newer ones).

This is the original log:

[2022-09-07T13:01:14.703Z] [Coverage] -> Found 'Plugins » design-library-plugin » master #93'
[2022-09-07T13:01:14.703Z] [Coverage] -> Using reference build 'Plugins/design-library-plugin/master #93'
[2022-09-07T13:01:14.703Z] [Coverage] -> Found reference result 'io.jenkins.plugins.coverage.CoverageAction@73999f2c'

But the build 93 has been deleted already.

Locally I get:

[Coverage] -> No reference build defined, falling back to previous build: '#1'
[Coverage] -> Found reference result in build '#1'
[Coverage] Calculating the code delta...

I'm not sure if the result is different if the reference build will be explicitly set.

And another difference: in a pull request there is an additional merge commit in the git repository that merges the HEAD of the PR with the latest commit of the master.

[uhafner]

I'm not sure if I fully understood createOldPathMapping, but the exception in #467 occurs because 2 new files are mapped to the same old file? Would it make sense to ensure in createOldPathMapping that not only the keys but also the values are unique?

[fo-code]

Yes, that causes the exception, I agree.
That would be a useful check to avoid the exception, but it does not fix the problem itself.

I think I was able to recreate the error just a few minutes ago, Do I have permissions to push the adjusted test case here? I also explained my guess there with comments.
I also can explain the behavior more detailed if you want, but we should do that in a call then.

[uhafner]

Yes, that causes the exception, I agree. That would be a useful check to avoid the exception, but it does not fix the problem itself.
I think I was able to recreate the error just a few minutes ago, Do I have permissions to push the adjusted test case here? I also explained my guess there with comments. I also can explain the behavior more detailed if you want, but we should do that in a call then.

I gave you write permissions to this repository, so you should have access to the existing branch. (Note that the test case is not ideal as it depends on a forked repository. Before merging it makes sense to create a stable one).

[fo-code]

Yes that is true. Nonetheless, I pushed the adjusted test case in order to get your opinion on this - note the comments within the test case.
I think you were right with the guess to try other commits.

Afterwards, we can adjust this test case if we are sure this is the error.

[uhafner]

Yes that is true. Nonetheless, I pushed the adjusted test case in order to get your opinion on this - note the comments within the test case. I think you were right with the guess to try other commits.

Yes, that probably is the actual problem (I think that I also needed to handle that case in GitCommitsCollector, when calculating the actual number of changed lines in a PR).

Afterwards, we can adjust this test case if we are sure this is the error.

I'm not sure if it is feasible to reproduce such a case without a pull request.

[fo-code]

Yes, that probably is the actual problem (I think that I also needed to handle that case in GitCommitsCollector, when calculating the actual number of changed lines in a PR).

Good, I will fix that and create a PR for the Git Forensics Plugin.

I'm not sure if it is feasible to reproduce such a case without a pull request.

I agree. And even if it is possible, it takes much effort. The developers which reported the error can test the fix and then we can close this PR without a merge in my opinion.

[fo-code]

@uhafner just to be sure, is there a possibility yet to get the merge commit in case of a pull request?
I read the code and I am not completely sure how the relevant lines work.
As I understand this, the actual head (the merge commit) is overwritten and only both the parents are stored. Also, getLatestCommit does not provide the merge commit, does it?
Here for example, the parent b22f9f0 is used for calculating the git delta instead of the merge commit 708f674 - using getLatestCommit.

[uhafner]

@uhafner just to be sure, is there a possibility yet to get the merge commit in case of a pull request?
I read the code and I am not completely sure how the relevant lines work.
As I understand this, the actual head (the merge commit) is overwritten and only both the parents are stored. Also, getLatestCommit does not provide the merge commit, does it?

First of all, I am checking in https://github.com/jenkinsci/git-forensics-plugin/blob/master/plugin/src/main/java/io/jenkins/plugins/forensics/git/reference/GitCheckoutListener.java#L124 for a merge request.

Here for example, the parent b22f9f0 is used for calculating the git delta instead of the merge commit 708f674 - using getLatestCommit.

Then I am checking if the head is a merge commit or not. I think not all cases are yet handled correctly, there is an open issue in Jira. The head can be a merge commit because the author merged the PR with master (your example) or if Jenkins created a temporary merge. Then the getLatestCommit should not include the merge.

But since I found no way to set up a good integration test, this code still might have bugs.

[fo-code]

@uhafner thanks for the explanation. I suggest the following solution:

• Prevent mismatches between code delta and jacoco data - Issue #467 #486 to prevent further unexpected exceptions
• Git Forensics #530 to include merge commits in commit records to solve this issue

Could you please review these approaches and we discuss them in the PRs?