Skip to content

Code coverage and bug fixes for bcelifier#171

Merged
garydgregory merged 9 commits intoapache:masterfrom
nbauma109:code-cov-bcelifier-20221120
Nov 20, 2022
Merged

Code coverage and bug fixes for bcelifier#171
garydgregory merged 9 commits intoapache:masterfrom
nbauma109:code-cov-bcelifier-20221120

Conversation

@nbauma109
Copy link
Copy Markdown
Contributor

@nbauma109 nbauma109 commented Nov 20, 2022

While doing coverage, I found a few bugs that needed to be fixed. I collected auxiliary jacoco execution reports for the exec(...) calls and merged them into the main one.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 20, 2022

Codecov Report

Merging #171 (d4b86a1) into master (467477c) will increase coverage by 2.35%.
The diff coverage is 96.66%.

@@             Coverage Diff              @@
##             master     #171      +/-   ##
============================================
+ Coverage     58.14%   60.49%   +2.35%     
- Complexity     3356     3560     +204     
============================================
  Files           363      363              
  Lines         15570    15619      +49     
  Branches       1921     1939      +18     
============================================
+ Hits           9053     9449     +396     
+ Misses         5633     5296     -337     
+ Partials        884      874      -10     
Impacted Files Coverage Δ
src/main/java/org/apache/bcel/generic/PUSH.java 56.52% <60.00%> (+45.58%) ⬆️
...main/java/org/apache/bcel/classfile/JavaClass.java 71.52% <100.00%> (+0.60%) ⬆️
...va/org/apache/bcel/generic/InstructionFactory.java 40.77% <100.00%> (+20.68%) ⬆️
src/main/java/org/apache/bcel/generic/LDC.java 80.85% <100.00%> (+10.63%) ⬆️
...rc/main/java/org/apache/bcel/util/BCELFactory.java 91.76% <100.00%> (+34.81%) ⬆️
src/main/java/org/apache/bcel/util/BCELifier.java 96.42% <100.00%> (+27.72%) ⬆️
src/main/java/org/apache/bcel/util/ClassPath.java 42.62% <0.00%> (-0.23%) ⬇️
...main/java/org/apache/bcel/generic/Instruction.java 89.32% <0.00%> (ø)
...n/java/org/apache/bcel/classfile/ConstantPool.java 73.18% <0.00%> (ø)
...c/main/java/org/apache/bcel/generic/MethodGen.java 68.39% <0.00%> (+0.43%) ⬆️
... and 48 more

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

@garydgregory
Copy link
Copy Markdown
Member

Hello @nbauma109

Thank you for your PR. There is a lot here but it feels like a mish-mash of different more or less related items.

For example, the PR adds the constructor org.apache.bcel.generic.PUSH.PUSH(ConstantPoolGen, ArrayType) but it is neither used nor tested. Either remove it, write a test, or use it and write a test for it. If there are missing APIs, I suggest you cover those in separate PRs. Mixing this new code here makes this PR harder to review.

Based on the title of the PR, I was expecting only tests, it's ok to have other work in here, the different pieces should be clearly documented in the PR description.

@nbauma109
Copy link
Copy Markdown
Contributor Author

nbauma109 commented Nov 20, 2022

The constructors org.apache.bcel.generic.PUSH.PUSH(ConstantPoolGen, ArrayType) and org.apache.bcel.generic.PUSH.PUSH(ConstantPoolGen, ObjectType) are not called directly by the code of the project but the BCELifier code generator creates calls to them. In target directory, a source file Java8Example2Creator.java is created with a call to :

il.append(new PUSH(_cp, new ArrayType(Type.STRING, 1)));

I generated a separate jacoco exec file for the execution of this code, which is merged into the main jacoco.exec, and coverage report is online:

src/main/java/org/apache/bcel/generic/PUSH.java

The codecov plugin is well made. The checks will fail if new code is not covered by tests.

@nbauma109 nbauma109 changed the title Code coverage bcelifier Code coverage and bug fixes for bcelifier Nov 20, 2022
@Override
public void visitRET(final RET i) {
printWriter.println("il.append(new RET(" + i.getIndex() + ")));");
printWriter.println("il.append(new RET(" + i.getIndex() + "));");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch!

@garydgregory garydgregory merged commit c96eb97 into apache:master Nov 20, 2022
@garydgregory
Copy link
Copy Markdown
Member

@nbauma109
Thank you for your reply. Merged.

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.

3 participants