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

Code coverage and bug fixes for bcelifier #171

Merged
merged 9 commits into from
Nov 20, 2022

Conversation

nbauma109
Copy link
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

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
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
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
@@ -299,7 +323,7 @@ public void visitLocalVariableInstruction(final LocalVariableInstruction i) {

@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
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
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