Repository: mockito/mockito
Issue: Improve exception message to hint at upgrading java minor version to latest
Labels: enhancement

Driven by #636


Issue comments:
Can be closed...


Pull request review comments:
Why would this be abstract? It has a private constructor anyways.

To keep it consistent, let's make these constant Strings too.

Space after `if`?

I would reverse this, to make the conditional equivalent to the name of the method. Very small nit though, but I had to think about it for a second :smile: 

It seems that these lines are untested. Are we able to come up with a test for this? E.g. why would we reorder these values.

replace 'visible' with 'non-private'? Users might not know what 'visible' is :)

AFAIR, StringJoiner always adds next line for each String argument. This means that if java is < 45, there will be extra EOLs. Perhaps it's fine, just wanted FYI.

This is a fantastic improvement, very Mockito-style :D We always strive to make the exception messages better, even a notch better. In the long run all those small improvements create a compound effect. We'll have fantastic error handling, crisp and clear exception messages and happy users!!!

FYI: Java 9 changes the standard version String and possibly other standard system properties. Let's make sure it works on Java 9 :)

Let's make sure this is always safe, regardless of the content of the system properties. Those properties are mutable AFAIK, and/or might change/be different with future versions of java or perhaps even jvm/jdk vendor.

Otherwise, love this PR :)

Yes I wanted an extra empty line before platform information (if the runtime is a java 8 version below update 45)

ok

At the moment this shouldn't be a problem. the parsing targets Java 8 runtime

http://openjdk.java.net/jeps/223

Because they are wrong in the message ;) 
The type to mock is not the class that the mockmaker created ;)

yes i forgot that, I was a bit in hurry for this PR, and wanted to push on the remote ;)
This was still a work in progress ;)

It's a usual practice to disallow instantiation.

Yes true, but that is what the private constructor guarantees too right? But leaving it in is fine too :)


Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java b/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java
index 907ebd0276..9d165f6899 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/ByteBuddyMockMaker.java
@@ -1,19 +1,18 @@
 package org.mockito.internal.creation.bytebuddy;
 
+import static org.mockito.internal.util.StringJoiner.join;
+import java.lang.reflect.Modifier;
 import org.mockito.exceptions.base.MockitoException;
 import org.mockito.internal.InternalMockHandler;
 import org.mockito.internal.configuration.plugins.Plugins;
 import org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.MockAccess;
 import org.mockito.internal.creation.instance.Instantiator;
+import org.mockito.internal.util.Platform;
 import org.mockito.invocation.MockHandler;
 import org.mockito.mock.MockCreationSettings;
 import org.mockito.mock.SerializableMode;
 import org.mockito.plugins.MockMaker;
 
-import java.lang.reflect.Modifier;
-
-import static org.mockito.internal.util.StringJoiner.join;
-
 public class ByteBuddyMockMaker implements MockMaker {
 
     private final CachingMockBytecodeGenerator cachingMockBytecodeGenerator;
@@ -37,8 +36,8 @@ public <T> T createMock(MockCreationSettings<T> settings, MockHandler handler) {
         } catch (ClassCastException cce) {
             throw new MockitoException(join(
                     "ClassCastException occurred while creating the mockito mock :",
-                    "  class to mock : " + describeClass(mockedProxyType),
-                    "  created class : " + describeClass(settings.getTypeToMock()),
+                    "  class to mock : " + describeClass(settings.getTypeToMock()),
+                    "  created class : " + describeClass(mockedProxyType),
                     "  proxy instance class : " + describeClass(mockInstance),
                     "  instance creation by : " + instantiator.getClass().getSimpleName(),
                     "",
@@ -51,7 +50,11 @@ public <T> T createMock(MockCreationSettings<T> settings, MockHandler handler) {
     }
 
     <T> Class<T> createProxyClass(MockFeatures<T> mockFeatures) {
-        return cachingMockBytecodeGenerator.get(mockFeatures);
+        try {
+            return cachingMockBytecodeGenerator.get(mockFeatures);
+        } catch (Exception bytecodeGenerationFailed) {
+            throw prettifyFailure(mockFeatures, bytecodeGenerationFailed);
+        }
     }
 
 
@@ -71,6 +74,27 @@ private <T> T ensureMockIsAssignableToMockedType(MockCreationSettings<T> setting
         return typeToMock.cast(mock);
     }
 
+    private RuntimeException prettifyFailure(MockFeatures<?> mockFeatures, Exception generationFailed) {
+        if (Modifier.isPrivate(mockFeatures.mockedType.getModifiers())) {
+            throw new MockitoException(join(
+                    "Mockito cannot mock this class: " + mockFeatures.mockedType + ".",
+                    "Most likely it is a private class that is not visible by Mockito",
+                    ""
+            ), generationFailed);
+        }
+        throw new MockitoException(join(
+                "Mockito cannot mock this class: " + mockFeatures.mockedType,
+                "",
+                "Mockito can only non-private & non-final classes.",
+                "If you're not sure why you're getting this error, please report to the mailing list.",
+                "",
+                Platform.isJava8BelowUpdate45() ? "Java 8 early builds have bugs that were addressed in Java 1.8.0_45, please update your JDK!\n" : "",
+                Platform.describe(),
+                "",
+                "Underlying exception : " + generationFailed
+        ), generationFailed);
+    }
+
     private static String describeClass(Class<?> type) {
         return type == null ? "null" : "'" + type.getCanonicalName() + "', loaded by classloader : '" + type.getClassLoader() + "'";
     }
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/CachingMockBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/CachingMockBytecodeGenerator.java
index 1093c37c87..461dec6168 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/CachingMockBytecodeGenerator.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/CachingMockBytecodeGenerator.java
@@ -1,12 +1,9 @@
 package org.mockito.internal.creation.bytebuddy;
 
-import org.mockito.exceptions.base.MockitoException;
-
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.SoftReference;
 import java.lang.ref.WeakReference;
-import java.lang.reflect.Modifier;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Collections;
@@ -15,8 +12,6 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
-import static org.mockito.internal.util.StringJoiner.join;
-
 class CachingMockBytecodeGenerator extends ReferenceQueue<ClassLoader> {
 
     private static final ClassLoader BOOT_LOADER = new URLClassLoader(new URL[0], null);
@@ -86,7 +81,7 @@ Class<?> getOrGenerateMockClass(MockFeatures<?> features) {
                 synchronized (features.mockedType) {
                     generatedMockClass = getMockClass(mockKey);
                     if(generatedMockClass == null) {
-                        generatedMockClass = generate(features);
+                        generatedMockClass = generator.generateMockClass(features);
                         generatedClassCache.put(mockKey, weak ? new WeakReference<Class<?>>(generatedMockClass) : new SoftReference<Class<?>>(generatedMockClass));
                     }
                 }
@@ -94,33 +89,6 @@ Class<?> getOrGenerateMockClass(MockFeatures<?> features) {
             return generatedMockClass;
         }
 
-        private <T> Class<? extends T> generate(MockFeatures<T> mockFeatures) {
-            try {
-                return generator.generateMockClass(mockFeatures);
-            } catch (Exception bytecodeGenerationFailed) {
-                throw prettifyFailure(mockFeatures, bytecodeGenerationFailed);
-            }
-        }
-
-        private RuntimeException prettifyFailure(MockFeatures<?> mockFeatures, Exception generationFailed) {
-            if (Modifier.isPrivate(mockFeatures.mockedType.getModifiers())) {
-                throw new MockitoException(join(
-                        "Mockito cannot mock this class: " + mockFeatures.mockedType + ".",
-                        "Most likely it is a private class that is not visible by Mockito",
-                        ""
-                ), generationFailed);
-            }
-            throw new MockitoException(join(
-                    "Mockito cannot mock this class: " + mockFeatures.mockedType,
-                    "",
-                    "Mockito can only mock visible & non-final classes.",
-                    "If you're not sure why you're getting this error, please report to the mailing list.",
-                    "",
-                    "Underlying exception : " + generationFailed),
-                    generationFailed
-            );
-        }
-
         // should be stored as a weak reference
         private static class MockKey<T> {
             private final String mockedType;
diff --git a/src/main/java/org/mockito/internal/util/Platform.java b/src/main/java/org/mockito/internal/util/Platform.java
new file mode 100644
index 0000000000..c899b20dab
--- /dev/null
+++ b/src/main/java/org/mockito/internal/util/Platform.java
@@ -0,0 +1,64 @@
+package org.mockito.internal.util;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public abstract class Platform {
+
+    private static final Pattern JAVA_8_RELEASE_VERSION_SCHEME = Pattern.compile("1\\.8\\.0_(\\d+)(?:-ea)?(?:-b\\d+)?");
+    private static final Pattern JAVA_8_DEV_VERSION_SCHEME = Pattern.compile("1\\.8\\.0b\\d+_u(\\d+)");
+    public static final String JAVA_VERSION = System.getProperty("java.specification.version");
+    public static final String JVM_VERSION = System.getProperty("java.runtime.version");
+    public static final String JVM_VENDOR = System.getProperty("java.vm.vendor");
+    public static final String JVM_VENDOR_VERSION = System.getProperty("java.vm.version");
+    public static final String JVM_NAME = System.getProperty("java.vm.name");
+    public static final String JVM_INFO = System.getProperty("java.vm.info");
+    public static final String OS_NAME = System.getProperty("os.name");
+    public static final String OS_VERSION = System.getProperty("os.version");
+
+    private Platform() {}
+
+    public static String describe() {
+        return String.format("Java               : %s\n" +
+                             "JVM vendor name    : %s\n" +
+                             "JVM vendor version : %s\n" +
+                             "JVM name           : %s\n" +
+                             "JVM version        : %s\n" +
+                             "JVM info           : %s\n" +
+                             "OS name            : %s\n" +
+                             "OS version         : %s\n",
+                             JAVA_VERSION,
+                             JVM_VENDOR,
+                             JVM_VENDOR_VERSION,
+                             JVM_NAME,
+                             JVM_VERSION,
+                             JVM_INFO,
+                             OS_NAME,
+                             OS_VERSION);
+    }
+
+    public static boolean isJava8BelowUpdate45() {
+        return isJava8BelowUpdate45(JVM_VERSION);
+    }
+
+    static boolean isJava8BelowUpdate45(String jvmVersion) {
+        Matcher matcher = JAVA_8_RELEASE_VERSION_SCHEME.matcher(jvmVersion);
+        if (matcher.matches()) {
+            int update = Integer.parseInt(matcher.group(1));
+            return update < 45;
+        }
+
+        matcher = JAVA_8_DEV_VERSION_SCHEME.matcher(jvmVersion);
+        if (matcher.matches()) {
+            int update = Integer.parseInt(matcher.group(1));
+            return update < 45;
+        }
+
+        matcher = Pattern.compile("1\\.8\\.0-b\\d+").matcher(jvmVersion);
+        if (matcher.matches()) {
+            return true;
+        }
+
+        return false;
+    }
+}


Test patch:
diff --git a/src/test/java/org/mockito/internal/util/PlatformTest.java b/src/test/java/org/mockito/internal/util/PlatformTest.java
new file mode 100644
index 0000000000..f146666620
--- /dev/null
+++ b/src/test/java/org/mockito/internal/util/PlatformTest.java
@@ -0,0 +1,98 @@
+package org.mockito.internal.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Test;
+
+public class PlatformTest {
+
+    @Test
+    public void const_are_initialized_from_system_properties() {
+        assertThat(Platform.JVM_VERSION).isEqualTo(System.getProperty("java.runtime.version"));
+        assertThat(Platform.JVM_INFO).isEqualTo(System.getProperty("java.vm.info"));
+        assertThat(Platform.JVM_NAME).isEqualTo(System.getProperty("java.vm.name"));
+        assertThat(Platform.JVM_VENDOR).isEqualTo(System.getProperty("java.vm.vendor"));
+        assertThat(Platform.JVM_VENDOR_VERSION).isEqualTo(System.getProperty("java.vm.version"));
+    }
+
+    @Test
+    public void should_parse_open_jdk_string() {
+        // Given
+        // Sources :
+        //  - http://www.oracle.com/technetwork/java/javase/versioning-naming-139433.html
+        //  - http://www.oracle.com/technetwork/java/javase/jdk7-naming-418744.html
+        //  - http://www.oracle.com/technetwork/java/javase/jdk8-naming-2157130.html
+        //  - http://stackoverflow.com/questions/35844985/how-do-we-get-sr-and-fp-of-ibm-jre-using-java
+        //  - http://www.ibm.com/support/knowledgecenter/SSYKE2_6.0.0/com.ibm.java.doc.user.win32.60/user/java_version_check.html
+        Map<String, Boolean> versions = new HashMap<String, Boolean>() {{
+            put("1.8.0_92-b14", false);
+            put("1.8.0-b24", true);
+            put("1.8.0_5", true);
+            put("1.8.0b5_u44", true);
+            put("1.8.0b5_u92", false);
+            put("1.7.0_4", false);
+            put("1.4.0_03-b04", false);
+            put("1.4.0_03-ea-b01", false);
+            put("pxi3270_27sr4-20160303_03 (SR4)", false);
+            put("pwi3260sr11-20120412_01 (SR11)", false);
+        }};
+
+        assertPlatformParsesCorrectlyVariousVersionScheme(versions);
+    }
+
+    @Test
+    public void should_parse_open_jdk9_string() {
+        // The tested method targets Java 8 but should be able to parse other Java version numbers including Java 9
+
+        // Given
+        // Sources :
+        //  - http://openjdk.java.net/jeps/223 (Java 9)
+        //
+        // System Property                 Existing      Proposed
+        // ------------------------------- ------------  --------
+        // Early Access
+        // java.runtime.version            1.9.0-ea-b73  9-ea+73
+        // java.vm.version                 1.9.0-ea-b73  9-ea+73
+        // java.specification.version      1.9           9
+        // java.vm.specification.version   1.9           9
+        //
+        // Major (GA)
+        // java.version                    1.9.0         9
+        // java.runtime.version            1.9.0-b100    9+100
+        // java.vm.version                 1.9.0-b100    9+100
+        // java.specification.version      1.9           9
+        // java.vm.specification.version   1.9           9
+        //
+        // Minor #1 (GA)
+        // java.version                    1.9.0_20      9.1.2
+        // java.runtime.version            1.9.0_20-b62  9.1.2+62
+        // java.vm.version                 1.9.0_20-b62  9.1.2+62
+        // java.specification.version      1.9           9
+        // java.vm.specification.version   1.9           9
+        //
+        // Security #1 (GA)
+        // java.version                    1.9.0_5       9.0.1
+        // java.runtime.version            1.9.0_5-b20   9.0.1+20
+        // java.vm.version                 1.9.0_5-b20   9.0.1+20
+        // java.specification.version      1.9           9
+        // java.vm.specification.version   1.9           9
+        //
+        Map<String, Boolean> versions = new HashMap<String, Boolean>() {{
+            put("9-ea+73", false);
+            put("9+100", false);
+            put("9.1.2+62", false);
+            put("9.0.1+20", false);
+        }};
+
+        assertPlatformParsesCorrectlyVariousVersionScheme(versions);
+    }
+
+
+    private void assertPlatformParsesCorrectlyVariousVersionScheme(Map<String, Boolean> versions) {
+        for (Map.Entry<String, Boolean> version : versions.entrySet()) {
+            assertThat(Platform.isJava8BelowUpdate45(version.getKey())).describedAs(version.getKey())
+                                                                       .isEqualTo(version.getValue());
+        }
+    }
+}