Repository: mockito/mockito
Issue: Whitebox.setInternalState for private static final fields
Labels: on hold

Hello,

Can you please add the ability for [Whitebox.setInternalState](https://github.com/mockito/mockito/blob/74e312b7a9236a43eb543bd3184607b75375bc40/src/main/java/org/mockito/internal/util/reflection/Whitebox.java#L22-L31) to be invokable on classes with private static final field?

``` java
Whitebox.setInternalState(ClassToTest.class, "LOG", mockLog);
```

I did not understand where is the best place to add a feature request. I hope this is the best place.

Thank you


Issue comments:
Hi! Why would you even want to do that? It sounds like a absolutely terrible idea to do that. Can you provide some use case?

Thank you for the fast reply!
I need to test that a class behaves in different ways if the log level is debug or not and so I need to mock the logger. If debug is enabled my class will do some heavy computation. The logger is fetched with the classic SLF4J API `LogFactory.getLog(ClassToTest.class)`.

I can use powermock to mock `LogFactory.getLog` but things get quite complicated quite fast since this is invoked only once when the class is loaded. A cleaner solution would be to set the mock logger before the test.

I am working on a low level logging API so I cannot rely on dependency injection.

@andreicristianpetcu I think setter injection would be better and cleaner. In your test, you set the logger via this method. In your default behavior you only set it in your constructor.

Loggers are per class and not per instance. If I make my log field not a static final field SonarQube starts to complain. I can add a false positive to SonarQube but the rest of the company uses this behavior.

Well you have two options:
1. Make the setter static too. In your `@Before` set the logger to your mock, but storing the original logger. In `@After` set the logger back to the original logger.
2. Deal with the deficiency of SonarQube and make this an exception. It is a tool to assist you during development, but not a holy grail imo.

I know that SonarQube is not a holy grail but I cannot change the SonarQube settings.
Would you refuse a pull request adding this feature? I imagine it is quite small.

Another solution is to wrap the logger in a class that you can inject. 

```
class LoggerProxy {

    private static final Logger log ....

    void debug(...) { log.debug(...); }
    void info(...) { log.info(...); }

}
```

and then in your class you can call

```
class YourClass {
    private final LoggerProxy proxy;

    YourClass(LoggerProxy proxy) {
        this.proxy = proxy;
    }

    void someMethod() {
        loggerProxy.debug(...); // <-- this you can stub
    }
}
```

I'd vote strongly against such a feature as allowing to set private final static fields. But that's just my vote ;) 

And then everybody's happy! You have cleaner code, Sonar is not violated - life's beautiful :)

I don't feel comfortable to write code specially to make my code testable.

Believe me I tried all possible options. I think setting the LOG field using reflection is the simplest. Right now we do it with a class who's purpose is to only set this field in several places. The only two options that I have:
- use this class;
- add the feature to upstream (in this case Mockito).

If you would not accept such a feature I will not add it but I think it adds value. I think setting private instance fields using reflection as it is currently implemented in Mockito is a bad practice but I use it anyway since there are some really rare cases where this is the best option. Setting private static final fields is the same as setting private instance fields: bad practice but needed from time to time.

Normally you shouldn't actually verify if stuff has been logged. If that's so important to you then IMO you should use good practices to achieve that solution. Maybe @bric3 or @szczepiq has some other opinion on this but I don't think that allowing more hacks to test a logger without injecting it through a constructor is a good idea. 

I understand you will not use this feature, but do you consider it harmful?
[Here is a StackOverflow](http://stackoverflow.com/a/2474242/1387563) solution for setting private static final field and I want to implement it the same way.

I consider it allowing people write bad code. But that's only my opinion.
If others feel that this is a good feature to add then I'm not going to
object.

I have to side with @marcingrzejszczak here. Adding this functionality will likely result in bad code because the production code is not testable. I do not see making code testable as a problem.

Ok then. I'll try to make a pull request tonight and I hope it gets accepted :)

So I can assume that we can close this

> Hi! Why would you even want to do that? It sounds like a absolutely terrible idea to do that. Can you provide some use case?

Could you please explain to me why using a static class variable is a terrible idea?
> I need to test that a class behaves in different ways if the log level is debug or not and so I need to mock the logger.

In this situation, you would set the logger's logging level to DEBUG from your logging framework, rather than mocking the logger.  For example in Logback:

```
LoggerContext loggerContext = (LoggerContext) LoggerFactory.getILoggerFactory();
ch.qos.logback.classic.Logger logger = loggerContext.getLogger(fullyQualifiedClassName);
logger.setLevel(Level.toLevel(logLevel));
```

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/util/reflection/Whitebox.java b/src/main/java/org/mockito/internal/util/reflection/Whitebox.java
index c63f59b87a..14ea30be98 100644
--- a/src/main/java/org/mockito/internal/util/reflection/Whitebox.java
+++ b/src/main/java/org/mockito/internal/util/reflection/Whitebox.java
@@ -5,31 +5,58 @@
 package org.mockito.internal.util.reflection;
 
 import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
 
 public class Whitebox {
 
-    public static Object getInternalState(Object target, String field) {
-        Class<?> c = target.getClass();
+    public static Object getInternalState(Object targetInstance, String fieldName) {
+        return getInternalState(targetInstance.getClass(), targetInstance, fieldName);
+    }
+
+    public static Object getInternalState(Class targetClass, String fieldName) {
+        return getInternalState(targetClass, null, fieldName);
+    }
+
+    private static Object getInternalState(Class<?> targetClass, Object targetInstance, String fieldName) {
         try {
-            Field f = getFieldFromHierarchy(c, field);
+            Field f = getFieldFromHierarchy(targetClass, fieldName);
             f.setAccessible(true);
-            return f.get(target);
+            return f.get(targetInstance);
         } catch (Exception e) {
             throw new RuntimeException("Unable to get internal state on a private field. Please report to mockito mailing list.", e);
         }
     }
 
-    public static void setInternalState(Object target, String field, Object value) {
-        Class<?> c = target.getClass();
+    public static void setInternalState(Object targetInstance, String fieldName, Object value) {
+        setInternalState(targetInstance.getClass(), targetInstance, fieldName, value);
+    }
+
+    public static void setInternalState(Class targetClass, String fieldName, Object value) {
+        setInternalState(targetClass, null, fieldName, value);
+    }
+
+    private static void setInternalState(Class targetClass, Object targetInstance, String fieldName, Object value) {
         try {
-            Field f = getFieldFromHierarchy(c, field);
+            Field f = getFieldFromHierarchy(targetClass, fieldName);
             f.setAccessible(true);
-            f.set(target, value);
+            removeFinalModifierIfPresent(f);
+            f.set(targetInstance, value);
         } catch (Exception e) {
             throw new RuntimeException("Unable to set internal state on a private field. Please report to mockito mailing list.", e);
         }
     }
 
+    private static void removeFinalModifierIfPresent(Field fieldToRemoveFinalFrom) throws IllegalAccessException, NoSuchFieldException {
+        int fieldModifiersMask = fieldToRemoveFinalFrom.getModifiers();
+        boolean isFinalModifierPresent = (fieldModifiersMask & Modifier.FINAL) == Modifier.FINAL;
+        if (isFinalModifierPresent) {
+            Field modifiersField = Field.class.getDeclaredField("modifiers");
+            modifiersField.setAccessible(true);
+            int fieldModifiersMaskWithoutFinal = fieldModifiersMask & ~Modifier.FINAL;
+            modifiersField.setInt(fieldToRemoveFinalFrom, fieldModifiersMaskWithoutFinal);
+        }
+    }
+
     private static Field getFieldFromHierarchy(Class<?> clazz, String field) {
         Field f = getField(clazz, field);
         while (f == null && clazz != Object.class) {


Test patch:
diff --git a/src/test/java/org/mockito/internal/util/reflection/DummyParentClassForTests.java b/src/test/java/org/mockito/internal/util/reflection/DummyParentClassForTests.java
index 6967c02c8a..2b5da62228 100644
--- a/src/test/java/org/mockito/internal/util/reflection/DummyParentClassForTests.java
+++ b/src/test/java/org/mockito/internal/util/reflection/DummyParentClassForTests.java
@@ -4,8 +4,13 @@
  */
 package org.mockito.internal.util.reflection;
 
+import java.util.logging.Logger;
+
 public class DummyParentClassForTests {
 
+    @SuppressWarnings("unused")//I know, I know. We're doing nasty reflection hacks here...
+    private static final Logger LOG = Logger.getLogger(DummyParentClassForTests.class.toString());
+
     @SuppressWarnings("unused")//I know, I know. We're doing nasty reflection hacks here...
     private String somePrivateField;
 }
diff --git a/src/test/java/org/mockito/internal/util/reflection/WhiteboxTest.java b/src/test/java/org/mockito/internal/util/reflection/WhiteboxTest.java
index add464c140..97a07ea530 100644
--- a/src/test/java/org/mockito/internal/util/reflection/WhiteboxTest.java
+++ b/src/test/java/org/mockito/internal/util/reflection/WhiteboxTest.java
@@ -5,10 +5,16 @@
 package org.mockito.internal.util.reflection;
 
 import org.junit.Test;
+import org.mockito.Mock;
 import org.mockitoutil.TestBase;
 
+import java.util.logging.Logger;
+
 public class WhiteboxTest extends TestBase {
 
+    @Mock
+    Logger mockLogger;
+
     @Test
     public void private_state() {
         //given
@@ -19,4 +25,13 @@ public void private_state() {
         Object internalState = Whitebox.getInternalState(dummy, "somePrivateField");
         assertEquals("cool!", internalState);
     }
+
+    @Test
+    public void private_static_final_state() {
+        //when
+        Whitebox.setInternalState(DummyParentClassForTests.class, "LOG", mockLogger);
+        //then
+        Object internalState = Whitebox.getInternalState(DummyParentClassForTests.class, "LOG");
+        assertEquals(mockLogger, internalState);
+    }
 }
\ No newline at end of file