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
Exclude from a report a part of bytecode that compiler generates for a try-with-resources #500
Merged
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
acda9ea
(WIP) Add filter for try-with-resources
Godin fa68b1e
(WIP) improve performance
Godin 779fca3
(wip) get rid of magic numbers
Godin 66e2856
(wip) remove unused method
Godin 4218461
(wip) add missing check
Godin fdf1985
(wip) add missing check
Godin 0cdb4c5
(wip) add missing check
Godin 890074c
(wip) extract filter for ECJ bytecode into separate class
Godin b118eac
(wip) add validation test
Godin 776c1dc
(wip) extract test of filter for ECJ bytecode into separate class
Godin 0fe450b
(wip) cleanup after split on two filters
Godin c244e71
(wip) use more meaningful name for local variable
Godin a1f22cc
(wip) remove useless skip of label of handler
Godin 3f0766f
(wip) consistency in names
Godin 1313daa
(wip) extract common part of filters into abstract class
Godin 2e4165c
(wip) use AbstractMatcher also for SynchronizedFilter
Godin 96bc5d4
Merge branch 'master'
Godin df07579
(wip) get rid of TODO in test
Godin 2240661
(wip) get rid of TODO
Godin 2929ed7
(wip) add comments to TryWithResourcesEcjFilterTest
Godin c9b3c28
(wip) add comments to TryWithResourcesJavacFilterTest
Godin 020a850
(wip) add comments to TryWithResourcesJavacFilter
Godin 61cf25f
(wip) make more declarative
Godin 5e622fb
(wip) add missing unit test
Godin da3b6e3
(wip) fix typo
Godin 297c45d
(wip) make test less fragile
Godin e29ec72
(wip) improve test
Godin File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
159 changes: 159 additions & 0 deletions
159
org.jacoco.core.test/src-java7/org/jacoco/core/test/filter/TryWithResourcesTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2009, 2017 Mountainminds GmbH & Co. KG and Contributors | ||
| * All rights reserved. This program and the accompanying materials | ||
| * are made available under the terms of the Eclipse Public License v1.0 | ||
| * which accompanies this distribution, and is available at | ||
| * http://www.eclipse.org/legal/epl-v10.html | ||
| * | ||
| * Contributors: | ||
| * Evgeny Mandrikov - initial API and implementation | ||
| * | ||
| *******************************************************************************/ | ||
| package org.jacoco.core.test.filter; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import org.jacoco.core.analysis.ICounter; | ||
| import org.jacoco.core.internal.Java9Support; | ||
| import org.jacoco.core.test.TargetLoader; | ||
| import org.jacoco.core.test.validation.ValidationTestBase; | ||
| import org.jacoco.core.test.filter.targets.TryWithResources; | ||
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Test of filtering of a bytecode that is generated for a try-with-resources | ||
| * statement. | ||
| */ | ||
| public class TryWithResourcesTest extends ValidationTestBase { | ||
|
|
||
| public TryWithResourcesTest() { | ||
| super("src-java7", TryWithResources.class); | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#test()} | ||
| */ | ||
| @Test | ||
| public void test() { | ||
| assertLine("test.before", ICounter.FULLY_COVERED); | ||
| // without filter next line covered partly: | ||
| assertLine("test.try", ICounter.FULLY_COVERED); | ||
| assertLine("test.open1", ICounter.FULLY_COVERED); | ||
| assertLine("test.open2", ICounter.FULLY_COVERED); | ||
| assertLine("test.open3", ICounter.FULLY_COVERED); | ||
| assertLine("test.body", ICounter.FULLY_COVERED); | ||
| // without filter next line has branches: | ||
| assertLine("test.close", ICounter.EMPTY); | ||
| assertLine("test.catch", ICounter.NOT_COVERED); | ||
| assertLine("test.finally", ICounter.PARTLY_COVERED); | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#test2()} | ||
| */ | ||
| @Test | ||
| public void test2() { | ||
| assertLine("test2.before", ICounter.FULLY_COVERED); | ||
| // without filter next line covered partly: | ||
| assertLine("test2.try", ICounter.FULLY_COVERED); | ||
| assertLine("test2.open1", ICounter.FULLY_COVERED); | ||
| assertLine("test2.open2", ICounter.FULLY_COVERED); | ||
| assertLine("test2.open3", ICounter.FULLY_COVERED); | ||
| assertLine("test2.body", ICounter.FULLY_COVERED); | ||
| // without filter next line has branches: | ||
| assertLine("test2.close", ICounter.EMPTY); | ||
| assertLine("test2.catch", ICounter.NOT_COVERED); | ||
| assertLine("test2.finally", ICounter.PARTLY_COVERED); | ||
| assertLine("test2.after", ICounter.FULLY_COVERED); | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#returnInBody()} | ||
| */ | ||
| @Test | ||
| public void returnInBody() { | ||
| // without filter next line covered partly: | ||
| assertLine("returnInBody.try", ICounter.FULLY_COVERED); | ||
| assertLine("returnInBody.open", ICounter.FULLY_COVERED); | ||
| // without filter next line has branches: | ||
| // TODO with javac 1.8.0_31 part of return instructions belong to close | ||
| // assertLine("returnInBody.close", ICounter.EMPTY); | ||
| assertLine("returnInBody.return", ICounter.FULLY_COVERED); | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#nested()} | ||
| */ | ||
| @Test | ||
| public void nested() { | ||
| // without filter next line covered partly: | ||
| assertLine("nested.try1", ICounter.FULLY_COVERED); | ||
| assertLine("nested.open1", ICounter.FULLY_COVERED); | ||
| assertLine("nested.catch1", ICounter.NOT_COVERED); | ||
|
|
||
| // without filter next line covered partly: | ||
| assertLine("nested.try2", ICounter.FULLY_COVERED); | ||
| assertLine("nested.body", ICounter.FULLY_COVERED); | ||
| assertLine("nested.catch2", ICounter.NOT_COVERED); | ||
| assertLine("nested.finally2", ICounter.PARTLY_COVERED); | ||
|
|
||
| // next lines not covered on exceptional path: | ||
| assertLine("nested.try3", ICounter.PARTLY_COVERED, 0, 0); | ||
| assertLine("nested.open3", ICounter.PARTLY_COVERED, 0, 0); | ||
| assertLine("nested.body3", ICounter.PARTLY_COVERED, 0, 0); | ||
| assertLine("nested.catch3", ICounter.NOT_COVERED); | ||
| assertLine("nested.finally3", ICounter.PARTLY_COVERED, 0, 0); | ||
|
|
||
| // without filter next lines have branches: | ||
| assertLine("nested.close3", ICounter.EMPTY); | ||
| assertLine("nested.close2", ICounter.EMPTY); | ||
| assertLine("nested.close1", ICounter.EMPTY); | ||
| } | ||
|
|
||
| /* | ||
| * Corner cases | ||
| */ | ||
|
|
||
| /** | ||
| * {@link TryWithResources#handwritten()} | ||
| */ | ||
| @Test | ||
| public void handwritten() { | ||
| if (isJDKCompiler) { | ||
| assertLine("handwritten", | ||
| /* partly when ECJ: */ICounter.EMPTY); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#empty()} | ||
| */ | ||
| @Test | ||
| public void empty() throws IOException { | ||
| final boolean java9 = Java9Support.isPatchRequired( | ||
| TargetLoader.getClassDataAsBytes(TryWithResources.class)); | ||
|
|
||
| assertLine("empty.try", ICounter.FULLY_COVERED, 0, 0); | ||
| assertLine("empty.open", ICounter.FULLY_COVERED); | ||
| // empty when EJC: | ||
| if (isJDKCompiler) { | ||
| if (java9) { | ||
| assertLine("empty.close", ICounter.FULLY_COVERED, 0, 0); | ||
| } else { | ||
| // branches with javac 7 and 8 | ||
| assertLine("empty.close", ICounter.PARTLY_COVERED); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@link TryWithResources#throwInBody()} | ||
| */ | ||
| @Test | ||
| public void throwInBody() throws Exception { | ||
| // not filtered | ||
| assertLine("throwInBody.try", ICounter.NOT_COVERED); | ||
| assertLine("throwInBody.close", ICounter.NOT_COVERED); | ||
| } | ||
|
|
||
| } | ||
172 changes: 172 additions & 0 deletions
172
org.jacoco.core.test/src-java7/org/jacoco/core/test/filter/targets/TryWithResources.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2009, 2017 Mountainminds GmbH & Co. KG and Contributors | ||
| * All rights reserved. This program and the accompanying materials | ||
| * are made available under the terms of the Eclipse Public License v1.0 | ||
| * which accompanies this distribution, and is available at | ||
| * http://www.eclipse.org/legal/epl-v10.html | ||
| * | ||
| * Contributors: | ||
| * Evgeny Mandrikov - initial API and implementation | ||
| * | ||
| *******************************************************************************/ | ||
| package org.jacoco.core.test.filter.targets; | ||
|
|
||
| import static org.jacoco.core.test.validation.targets.Stubs.nop; | ||
|
|
||
| import java.io.Closeable; | ||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * This test target is a try-with-resources statement. | ||
| */ | ||
| public class TryWithResources { | ||
|
|
||
| private static class Resource implements Closeable { | ||
| @Override | ||
| public void close() { | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Closing performed using {@link org.objectweb.asm.Opcodes#INVOKEVIRTUAL} | ||
| * or {@link org.objectweb.asm.Opcodes#INVOKEINTERFACE} depending on a class | ||
| * of resource. | ||
| */ | ||
| private static Object test() throws Exception { | ||
| nop(); // $line-test.before$ | ||
| try ( // $line-test.try$ | ||
| Resource r1 = new Resource(); // $line-test.open1$ | ||
| Closeable r2 = new Resource(); // $line-test.open2$ | ||
| AutoCloseable r3 = new Resource() // $line-test.open3$ | ||
| ) { | ||
| return read(r1, r2, r3); // $line-test.body$ | ||
| } // $line-test.close$ | ||
| catch (Exception e) { | ||
| nop(); // $line-test.catch$ | ||
| throw e; | ||
| } finally { | ||
| nop(); // $line-test.finally$ | ||
| } | ||
| } | ||
|
|
||
| private static void test2() throws Exception { | ||
| nop(); // $line-test2.before$ | ||
| try ( // $line-test2.try$ | ||
| Resource r1 = new Resource(); // $line-test2.open1$ | ||
| Closeable r2 = new Resource(); // $line-test2.open2$ | ||
| AutoCloseable r3 = new Resource() // $line-test2.open3$ | ||
| ) { | ||
| read(r1, r2, r3); // $line-test2.body$ | ||
| } // $line-test2.close$ | ||
| catch (Exception e) { | ||
| nop(); // $line-test2.catch$ | ||
| } finally { | ||
| nop(); // $line-test2.finally$ | ||
| } | ||
| nop(); // $line-test2.after$ | ||
| } | ||
|
|
||
| private static Object returnInBody() throws IOException { | ||
| try ( // $line-returnInBody.try$ | ||
| Closeable r = new Resource() // $line-returnInBody.open$ | ||
| ) { | ||
| return read(r); // $line-returnInBody.return$ | ||
| } // $line-returnInBody.close$ | ||
| } | ||
|
|
||
| private static void nested() { | ||
| try ( // $line-nested.try1$ | ||
| Resource r1 = new Resource() // $line-nested.open1$ | ||
| ) { | ||
|
|
||
| try ( // $line-nested.try2$ | ||
| Resource r2 = new Resource() // $line-nested.open2$ | ||
| ) { | ||
| nop(r1.toString() + r2.toString()); // $line-nested.body$ | ||
| } // $line-nested.close2$ | ||
| catch (Exception e) { | ||
| nop(); // $line-nested.catch2$ | ||
| } finally { | ||
| nop(); // $line-nested.finally2$ | ||
| } | ||
|
|
||
| } // $line-nested.close1$ | ||
| catch (Exception e) { | ||
| nop(); // $line-nested.catch1$ | ||
| } finally { | ||
|
|
||
| try ( // $line-nested.try3$ | ||
| Resource r2 = new Resource() // $line-nested.open3$ | ||
| ) { | ||
| nop(r2); // $line-nested.body3$ | ||
| } // $line-nested.close3$ | ||
| catch (Exception e) { | ||
| nop(); // $line-nested.catch3$ | ||
| } finally { | ||
| nop(); // $line-nested.finally3$ | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| private static Object read(Object r1, Object r2, Object r3) { | ||
| return r1.toString() + r2.toString() + r3.toString(); | ||
| } | ||
|
|
||
| private static Object read(Object r1) { | ||
| return r1.toString(); | ||
| } | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| test(); | ||
| test2(); | ||
| returnInBody(); | ||
| nested(); | ||
|
|
||
| empty(); | ||
| handwritten(); | ||
| } | ||
|
|
||
| /* | ||
| * Corner cases | ||
| */ | ||
|
|
||
| private static void empty() throws Exception { | ||
| try ( // $line-empty.try$ | ||
| Closeable r = new Resource() // $line-empty.open$ | ||
| ) { | ||
| } // $line-empty.close$ | ||
| } | ||
|
|
||
| private static void handwritten() throws IOException { | ||
| Closeable r = new Resource(); | ||
| Throwable primaryExc = null; | ||
| try { | ||
| nop(r); | ||
| } catch (Throwable t) { | ||
| primaryExc = t; | ||
| throw t; | ||
| } finally { | ||
| if (r != null) { // $line-handwritten$ | ||
| if (primaryExc != null) { | ||
| try { | ||
| r.close(); | ||
| } catch (Throwable suppressedExc) { | ||
| primaryExc.addSuppressed(suppressedExc); | ||
| } | ||
| } else { | ||
| r.close(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static void throwInBody() throws IOException { | ||
| try ( // $line-throwInBody.try$ | ||
| Closeable r = new Resource()) { | ||
| nop(r); | ||
| throw new RuntimeException(); | ||
| } // $line-throwInBody.close$ | ||
| } | ||
|
|
||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of
we correctly find instructions, but javac 7 as well as javac 8 up to 8u77 (tested public updates) generates:
while javac starting from 8u92 (notice additional
LINENUMBERbeforeALOAD) as well as latest 9 EA:Looks like this relates to JDK-8134759 (log of bisect - bisect.txt)
There is no such problem when
finallyis present, but this case is covered by another existing test.So I'm wondering what can we do with this test?
Travis provides JDK 8u31 by default and we also test JDK 8 EA. Can imagine that for the bytecode version 7 generated by javac we will be testing old behavior, while will be testing and using updated stable JDK 8 in Travis as well as locally.
But maybe @marchof you have some other thoughts/ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Parse
java.version, and then skip or adjust the asserts? We have precedent inBadCycleInterfaceTest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjkail
java.versionis about version of JRE andBadCycleInterfaceTestis exactly about runtime behavior. This will work here if we assume that execution and compilation of tests use the same JDK version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin You're right. Is that too fragile? If so, the only other option that comes to mind is to use ASM to detect the missing line number. That would weaken the test a bit, but perhaps it's better than nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin @bjkail Maybe our integration tests for filtering are too specific. What if we just assert that our filtering examples do not produce any uncovered lines and branches? We already have plenty of validation test for expected covered/missed lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof Excuse me for the delay with reply - was at DevoxFR, where BTW session "Java Code Coverage Mechics" was a quite success.
On a subject:
@bjkail
java.versionmaybe not so fragile.is fragile in terms that it allows compiler to regress on this.
@marchof
I personally like such specificity for the following reasons
Apparently not for such tricky control flows as shown here 😉
Do you see any troubles to simply bump version of javac 8 up to latest update as in my initial proposal - "past is past"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin We have a large user base out there where older JDK versions still might be in use. The primary purpose of the integration tests for filters should be to ensure that the filters fulfill their purpose. I would imagine filter integration tests consisting of source code only (maybe with annotated lines for the intentionally un-covered ones).
My primary concern is maintainability. If you think we can handle the specific checks in the long run, I can live with the current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof
I haven't said that we should stop testing all old JDK versions - only range of JDK 8 till 8u92.
JDK 7 will be testing the same bytecode that generates JDK 8 from this range. And this range was shown to be working with other existing test cases.
I wish that we run tests against range of all public updates of all supported JDK versions, but this is quite overkill and we are not doing this anyway 😉
Some other options:
But all this quite out of scope of this PR. So for now I excluded assertion on this line only for bytecode version 8 - this is at least better that current TODO-comment and we can come back to this discussion later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arr, in fact even this is fragile as one can ask generation of bytecode version 7 or 8 using JDK 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimented with this over past weeks and few times experienced fragility on
mvn clean install -Dbytecode.version=1.7using JDK 8, so finally in favor of @bjkail suggestion to assume that execution and compilation of tests is done with same JDK.✅