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.

Sign up for GitHub

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

Jump to bottom

Include JaCoCo version in instrumentation/analysis exception messages #1217

Merged
merged 4 commits into from Feb 3, 2022
Merged

Include JaCoCo version in instrumentation/analysis exception messages #1217

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
161deee
Add source commit ID to JaCoCo constants
marchof Sep 5, 2021
9e736bf
Add JaCoCo version to instrumentation and analysis errors
marchof Sep 5, 2021
f9e49cf
Apply suggestions from code review
marchof Feb 3, 2022
6b76303
Update changelog
Godin Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/CoverageTransformerTest.java
View file
Open in desktop
Expand Up @@ -12,7 +12,6 @@
*******************************************************************************/
package org.jacoco.agent.rt.internal;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -215,11 +214,11 @@ public void testTransformFailure() {
protectionDomain, null);
fail("IllegalClassFormatException expected.");
} catch (IllegalClassFormatException e) {
assertEquals("Error while instrumenting org.jacoco.Sample.",
e.getMessage());
assertTrue(e.getMessage(), e.getMessage()
.startsWith("Error while instrumenting org.jacoco.Sample"));
marchof marked this conversation as resolved.
Show resolved Hide resolved
}
recorder.assertException(IllegalClassFormatException.class,
"Error while instrumenting org.jacoco.Sample.",
"Error while instrumenting org.jacoco.Sample",
marchof marked this conversation as resolved.
Show resolved Hide resolved
IOException.class);
recorder.clear();
}
Expand Down
3 changes: 2 additions & 1 deletion org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/ExceptionRecorder.java
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

/**
* {@link IExceptionLogger} implementation for testing purposes.
Expand Down Expand Up @@ -50,7 +51,7 @@ public void assertException(final Class<? extends Throwable> exceptionType,
public void assertException(final Class<? extends Throwable> exceptionType,
final String message, final Class<? extends Throwable> causeType) {
assertEquals(exceptionType, this.exceptionType);
assertEquals(message, this.message);
assertTrue(this.message, this.message.startsWith(message));
marchof marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(causeType, this.causeType);
}

Expand Down
11 changes: 11 additions & 0 deletions org.jacoco.core.test/src/org/jacoco/core/JaCoCoTest.java
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
*******************************************************************************/
package org.jacoco.core;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

import org.junit.Test;
Expand All @@ -26,6 +27,16 @@ public void testVERSION() {
assertNotNull(JaCoCo.VERSION);
}

@Test
public void testCOMMITID() {
assertNotNull(JaCoCo.COMMITID);
}

@Test
public void testCOMMITID_SHORT() {
assertEquals(7, JaCoCo.COMMITID_SHORT.length());
}

@Test
public void testHOMEURL() {
assertNotNull(JaCoCo.HOMEURL);
Expand Down
34 changes: 19 additions & 15 deletions org.jacoco.core.test/src/org/jacoco/core/analysis/AnalyzerTest.java
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@
import java.util.zip.ZipInputStream;
import java.util.zip.ZipOutputStream;

import org.jacoco.core.JaCoCo;
import org.jacoco.core.data.ExecutionDataStore;
import org.jacoco.core.internal.Pack200Streams;
import org.jacoco.core.internal.data.CRC64;
Expand Down Expand Up @@ -136,8 +137,7 @@ public void analyzeClass_should_throw_exception_for_unsupported_class_file_versi
analyzer.analyzeClass(bytes, "UnsupportedVersion");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while analyzing UnsupportedVersion.",
e.getMessage());
assertExceptionMessage("UnsupportedVersion", e);
assertEquals("Unsupported class file major version 64",
e.getCause().getMessage());
}
Expand All @@ -164,7 +164,7 @@ public void testAnalyzeClassIdMatch() throws IOException {
final byte[] bytes = TargetLoader
.getClassDataAsBytes(AnalyzerTest.class);
executionData.get(Long.valueOf(CRC64.classId(bytes)),
"org/jacoco/core/analysis/AnalyzerTest", 200);
"org/jacoco/core/analysis/AnalyzerTest", 400);
analyzer.analyzeClass(bytes, "Test");
assertFalse(classes.get("org/jacoco/core/analysis/AnalyzerTest")
.isNoMatch());
Expand All @@ -173,7 +173,7 @@ public void testAnalyzeClassIdMatch() throws IOException {
@Test
public void testAnalyzeClassNoIdMatch() throws IOException {
executionData.get(Long.valueOf(0),
"org/jacoco/core/analysis/AnalyzerTest", 200);
"org/jacoco/core/analysis/AnalyzerTest", 400);
analyzer.analyzeClass(
TargetLoader.getClassDataAsBytes(AnalyzerTest.class), "Test");
assertTrue(classes.get("org/jacoco/core/analysis/AnalyzerTest")
Expand All @@ -189,7 +189,7 @@ public void testAnalyzeClass_Broken() throws IOException {
analyzer.analyzeClass(brokenclass, "Broken.class");
fail("expected exception");
} catch (IOException e) {
assertEquals("Error while analyzing Broken.class.", e.getMessage());
assertExceptionMessage("Broken.class", e);
}
}

Expand All @@ -209,7 +209,7 @@ public void testAnalyzeClass_BrokenStream() throws IOException {
analyzer.analyzeClass(new BrokenInputStream(), "BrokenStream");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while analyzing BrokenStream.", e.getMessage());
assertExceptionMessage("BrokenStream", e);
}
}

Expand All @@ -224,8 +224,7 @@ public void analyzeAll_should_throw_exception_for_unsupported_class_file_version
"UnsupportedVersion");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while analyzing UnsupportedVersion.",
e.getMessage());
assertExceptionMessage("UnsupportedVersion", e);
assertEquals("Unsupported class file major version 64",
e.getCause().getMessage());
}
Expand Down Expand Up @@ -274,7 +273,7 @@ public void testAnalyzeAll_Broken() throws IOException {
analyzer.analyzeAll(new BrokenInputStream(), "Test");
fail("expected exception");
} catch (IOException e) {
assertEquals("Error while analyzing Test.", e.getMessage());
assertExceptionMessage("Test", e);
}
}

Expand All @@ -289,7 +288,7 @@ public void testAnalyzeAll_BrokenGZ() {
analyzer.analyzeAll(new ByteArrayInputStream(buffer), "Test.gz");
fail("expected exception");
} catch (IOException e) {
assertEquals("Error while analyzing Test.gz.", e.getMessage());
assertExceptionMessage("Test.gz", e);
}
}

Expand Down Expand Up @@ -333,7 +332,7 @@ public void testAnalyzeAll_BrokenPack200() {
"Test.pack200");
fail("expected exception");
} catch (IOException e) {
assertEquals("Error while analyzing Test.pack200.", e.getMessage());
assertExceptionMessage("Test.pack200", e);
}
}

Expand Down Expand Up @@ -380,7 +379,7 @@ public void testAnalyzeAll_BrokenZip() {
analyzer.analyzeAll(new ByteArrayInputStream(buffer), "Test.zip");
fail("expected exception");
} catch (IOException e) {
assertEquals("Error while analyzing Test.zip.", e.getMessage());
assertExceptionMessage("Test.zip", e);
}
}

Expand Down Expand Up @@ -431,9 +430,8 @@ public void testAnalyzeAll_BrokenClassFileInZip() throws IOException {
"test.zip");
fail("expected exception");
} catch (IOException e) {
assertEquals(
"Error while analyzing test.zip@org/jacoco/core/analysis/AnalyzerTest.class.",
e.getMessage());
assertExceptionMessage(
"test.zip@org/jacoco/core/analysis/AnalyzerTest.class", e);
}
}

Expand All @@ -452,4 +450,10 @@ private void assertClasses(String... classNames) {
classes.keySet());
}

private void assertExceptionMessage(String name, Exception ex) {
String expected = "Error while analyzing " + name + " with JaCoCo "
+ JaCoCo.VERSION + "/" + JaCoCo.COMMITID_SHORT + ".";
assertEquals(expected, ex.getMessage());
}

}
40 changes: 19 additions & 21 deletions org.jacoco.core.test/src/org/jacoco/core/instr/InstrumenterTest.java
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@
import java.util.zip.ZipInputStream;
import java.util.zip.ZipOutputStream;

import org.jacoco.core.JaCoCo;
import org.jacoco.core.analysis.AnalyzerTest;
import org.jacoco.core.internal.Pack200Streams;
import org.jacoco.core.internal.data.CRC64;
Expand Down Expand Up @@ -127,8 +128,7 @@ public void instrument_should_throw_exception_for_unsupported_class_file_version
instrumenter.instrument(bytes, "UnsupportedVersion");
fail("exception expected");
} catch (final IOException e) {
assertEquals("Error while instrumenting UnsupportedVersion.",
e.getMessage());
assertExceptionMessage("UnsupportedVersion", e);
assertEquals("Unsupported class file major version 64",
e.getCause().getMessage());
}
Expand Down Expand Up @@ -156,8 +156,7 @@ public void testInstrumentBrokenClass1() throws IOException {
instrumenter.instrument(brokenclass, "Broken.class");
fail();
} catch (IOException e) {
assertEquals("Error while instrumenting Broken.class.",
e.getMessage());
assertExceptionMessage("Broken.class", e);
}
}

Expand All @@ -178,8 +177,7 @@ public void testInstrumentBrokenStream() {
instrumenter.instrument(new BrokenInputStream(), "BrokenStream");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while instrumenting BrokenStream.",
e.getMessage());
assertExceptionMessage("BrokenStream", e);
}
}

Expand All @@ -194,8 +192,7 @@ public void testInstrumentBrokenStream2() {
new ByteArrayOutputStream(), "BrokenStream");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while instrumenting BrokenStream.",
e.getMessage());
assertExceptionMessage("BrokenStream", e);
}
}

Expand Down Expand Up @@ -230,8 +227,7 @@ public void instrumentAll_should_throw_exception_for_unsupported_class_file_vers
new ByteArrayOutputStream(), "UnsupportedVersion");
fail("exception expected");
} catch (final IOException e) {
assertEquals("Error while instrumenting UnsupportedVersion.",
e.getMessage());
assertExceptionMessage("UnsupportedVersion", e);
assertEquals("Unsupported class file major version 64",
e.getCause().getMessage());
}
Expand Down Expand Up @@ -297,7 +293,7 @@ public void testInstrumentAll_Broken() {
new ByteArrayOutputStream(), "Broken");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while instrumenting Broken.", e.getMessage());
assertExceptionMessage("Broken", e);
}
}

Expand All @@ -324,7 +320,7 @@ public int read() throws IOException {
instrumenter.instrumentAll(inputStream, new ByteArrayOutputStream(),
"Broken");
} catch (IOException e) {
assertEquals("Error while instrumenting Broken.", e.getMessage());
assertExceptionMessage("Broken", e);
}
}

Expand All @@ -346,7 +342,7 @@ public void testInstrumentAll_BrokenZip() {
new ByteArrayOutputStream(), "Test.zip");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while instrumenting Test.zip.", e.getMessage());
assertExceptionMessage("Test.zip", e);
}
}

Expand All @@ -371,9 +367,7 @@ public void testInstrumentAll_BrokenZipEntry() throws IOException {
new ByteArrayOutputStream(), "broken.zip");
fail("exception expected");
} catch (IOException e) {
assertEquals(
"Error while instrumenting broken.zip@brokenentry.txt.",
e.getMessage());
assertExceptionMessage("broken.zip@brokenentry.txt", e);
}
}

Expand All @@ -394,8 +388,7 @@ public void testInstrumentAll_BrokenClassFileInZip() throws IOException {
"test.zip");
fail();
} catch (IOException e) {
assertEquals("Error while instrumenting test.zip@Test.class.",
e.getMessage());
assertExceptionMessage("test.zip@Test.class", e);
}
}

Expand All @@ -412,7 +405,7 @@ public void testInstrumentAll_BrokenGZ() {
new ByteArrayOutputStream(), "Test.gz");
fail("exception expected");
} catch (IOException e) {
assertEquals("Error while instrumenting Test.gz.", e.getMessage());
assertExceptionMessage("Test.gz", e);
}
}

Expand Down Expand Up @@ -462,8 +455,7 @@ public void testInstrumentAll_BrokenPack200() {
instrumenter.instrumentAll(new ByteArrayInputStream(buffer),
new ByteArrayOutputStream(), "Test.pack200");
} catch (IOException e) {
assertEquals("Error while instrumenting Test.pack200.",
e.getMessage());
assertExceptionMessage("Test.pack200", e);
}
}

Expand Down Expand Up @@ -516,4 +508,10 @@ public void testInstrumentAll_KeepSignatures() throws IOException {
assertNull(zipin.getNextEntry());
}

private void assertExceptionMessage(String name, Exception ex) {
String expected = "Error while instrumenting " + name + " with JaCoCo "
+ JaCoCo.VERSION + "/" + JaCoCo.COMMITID_SHORT + ".";
assertEquals(expected, ex.getMessage());
}

}
12 changes: 11 additions & 1 deletion org.jacoco.core/src/org/jacoco/core/JaCoCo.java
View file
Open in desktop
Expand Up @@ -19,9 +19,17 @@
*/
public final class JaCoCo {

/** Qualified build version of the JaCoCo core library. */
/** Version of JaCoCo core. */
Copy link
Member

Choose a reason for hiding this comment

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

@marchof this is indeed qualified version, i.e. in https://repo1.maven.org/maven2/org/jacoco/org.jacoco.core/0.8.7/org.jacoco.core-0.8.7.jar

VERSION=0.8.7.202105040129

So I'm wondering whether change of javadoc here implies that you expect(ed) to see

Error while analyzing Example.class with JaCoCo 0.8.8/95a2b3e2.

instead of

Error while analyzing Example.class with JaCoCo 0.8.8.202202010859/95a2b3e2.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups my bad. I was wrongly assuming we have the plain version here. From my point of view the plain version would be enough here. What do you think, can we change this to project.version?

If we don't want to change this in this PR the qualified version is also good for me.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think, can we change this to project.version?

Well I have mixed feelings:

Maybe not a big deal to change it, however strictly speaking it is part of our public API on it's own and a return value of Agent API org.jacoco.agent.rt.IAgent.getVersion, also it is shown by version command in CLI and always displayed in footer (right-bottom corner) of html reports.

And on one side, since qualifier is build timestamp, maybe nice that people will see how old version they are using. On the other side, since their configuration files use unqualified version, presence of qualifier might be confusing.

We can introduce VERSION_SHORT by analogy with COMMITID_SHORT, but maybe this is an overkill.

If we don't want to change this in this PR the qualified version is also good for me.

So let's keep it as is for now.

I was wrongly assuming we have the plain version here.

I'll add back word "Qualified" so that it can again remind us next time 😉

public static final String VERSION;

/** Commit ID of the source tree of JaCoCo core. */
public static final String COMMITID;

/**
* Shortened (7 digit) commit ID of the source tree of JaCoCo core.
*/
public static final String COMMITID_SHORT;

/** Absolute URL of the current JaCoCo home page */
public static final String HOMEURL;

Expand All @@ -32,6 +40,8 @@ public final class JaCoCo {
final ResourceBundle bundle = ResourceBundle
.getBundle("org.jacoco.core.jacoco");
VERSION = bundle.getString("VERSION");
COMMITID = bundle.getString("COMMITID");
COMMITID_SHORT = COMMITID.substring(0, 7);
HOMEURL = bundle.getString("HOMEURL");
RUNTIMEPACKAGE = bundle.getString("RUNTIMEPACKAGE");
Comment on lines +44 to 46
Copy link
Member

Choose a reason for hiding this comment

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

@marchof BTW fun fact:
COMMITID_SHORT is the first 7 digits of COMMITID computed at runtime,
RUNTIMEPACKAGE is the last 7 digits of COMMITID computed at build time
😆

}
Expand Down
4 changes: 3 additions & 1 deletion org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipInputStream;

import org.jacoco.core.JaCoCo;
import org.jacoco.core.data.ExecutionData;
import org.jacoco.core.data.ExecutionDataStore;
import org.jacoco.core.internal.ContentTypeDetector;
Expand Down Expand Up @@ -160,7 +161,8 @@ public void analyzeClass(final InputStream input, final String location)
private IOException analyzerError(final String location,
final Exception cause) {
final IOException ex = new IOException(
String.format("Error while analyzing %s.", location));
String.format("Error while analyzing %s with JaCoCo %s/%s.",
location, JaCoCo.VERSION, JaCoCo.COMMITID_SHORT));
ex.initCause(cause);
return ex;
}
Expand Down
4 changes: 3 additions & 1 deletion org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
import java.util.zip.ZipInputStream;
import java.util.zip.ZipOutputStream;

import org.jacoco.core.JaCoCo;
import org.jacoco.core.internal.ContentTypeDetector;
import org.jacoco.core.internal.InputStreams;
import org.jacoco.core.internal.Pack200Streams;
Expand Down Expand Up @@ -158,7 +159,8 @@ public void instrument(final InputStream input, final OutputStream output,
private IOException instrumentError(final String name,
final Exception cause) {
final IOException ex = new IOException(
String.format("Error while instrumenting %s.", name));
String.format("Error while instrumenting %s with JaCoCo %s/%s.",
name, JaCoCo.VERSION, JaCoCo.COMMITID_SHORT));
ex.initCause(cause);
return ex;
}
Expand Down
1 change: 1 addition & 0 deletions org.jacoco.core/src/org/jacoco/core/jacoco.properties
View file
Open in desktop
@@ -1,3 +1,4 @@
VERSION=${qualified.bundle.version}
COMMITID=${buildNumber}
Copy link
Member

Choose a reason for hiding this comment

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

@marchof hope people won't be faking this value in forks 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Well ... 🤷‍♂️

HOMEURL=${jacoco.home.url}
RUNTIMEPACKAGE=${jacoco.runtime.package.name}