Skip to content

Fix test failure with static mocking by replacing PowerMock with Mockito #4254

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

Merged
merged 4 commits into from
Dec 10, 2023
Merged

Fix test failure with static mocking by replacing PowerMock with Mockito #4254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b9708a
draft only
JooHyukKim Dec 10, 2023
7381047
Fix version
JooHyukKim Dec 10, 2023
d6e4389
Put back tests and fix them
JooHyukKim Dec 10, 2023
b8a62ec
Upgrade byte buddy
JooHyukKim Dec 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,6 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-core</artifactId>
<version>${version.powermock}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-module-junit4</artifactId>
<version>${version.powermock}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-api-mockito2</artifactId>
<version>${version.powermock}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava-testlib</artifactId>
Expand All @@ -142,6 +124,27 @@
<version>0.16</version>
<scope>test</scope>
</dependency>
<!-- Since 2.17, started using Mockito instead of Powermock -->
<!-- For testing with static methods -->
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>4.11.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>4.11.0</version>
<scope>test</scope>
</dependency>
<!-- We need to specify later Bytebuddy version because Mockito 4.11.0 depends on earlier Bytebuddy which
does not support JDK 21, since 2.17-->
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.14.9</version>
</dependency>
Copy link
Member Author

@JooHyukKim JooHyukKim Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Phew, took a while to fix the JDK 21 failure. The problem was that

  • We need Mockito 4.x because Mockito 5.x baseline jdk is 11 (where as we support jdk 8)
  • Mockito 4.x depends on earlier ByteBuddy version that does not support JDK 21

This measure seems like necessary cost of supporting a wide range of JDK versions.

Copy link
Member

Choose a reason for hiding this comment

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

Fun fun. Yeah, things will become more challenging going forward, no doubt.

Copy link

Choose a reason for hiding this comment

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

@JooHyukKim @cowtowncoder any reason for ByteBuddy not being scoped to test?

Copy link
Member

Choose a reason for hiding this comment

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

Oh F*CK. This is bad. How did we miss this. :-(

Copy link
Member

Choose a reason for hiding this comment

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

just release note it, no need to release 2.17.1 immediately - it is easy to work around (by excluding it in your build) and most people won't even notice the extra jar - needs to be fixed but not something to worry too much about

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this need not force release of 2.17.1

</dependencies>

<!-- Alas, need to include snapshot reference since otherwise can not find
Expand Down
199 changes: 100 additions & 99 deletions src/test/java/com/fasterxml/jackson/databind/deser/lazy/LazyIgnoralForNumbers3730Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,23 @@
package com.fasterxml.jackson.databind.deser.lazy;

import java.math.BigDecimal;

import org.mockito.MockedStatic;
import org.mockito.Mockito;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonUnwrapped;
import com.fasterxml.jackson.core.io.NumberInput;

import com.fasterxml.jackson.databind.BaseMapTest;
import com.fasterxml.jackson.databind.DatabindException;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.json.JsonMapper;

import org.junit.Ignore;
import org.junit.runner.RunWith;
import org.mockito.Mockito;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import static org.powermock.api.mockito.PowerMockito.*;

import java.math.BigDecimal;
import static org.mockito.Mockito.mockStatic;

/**
* Tests to verify that skipping of unknown/unmapped works such that
* "expensive" numbers (all floating-point, {@code BigInteger}) is avoided.
*/
@RunWith(PowerMockRunner.class)
@PrepareForTest(fullyQualifiedNames = "com.fasterxml.jackson.core.io.NumberInput")
@Ignore("Powermock stopped working with NumberUtil refactoring see [databind#4250]")
public class LazyIgnoralForNumbers3730Test extends BaseMapTest
{
static class ExtractFieldsNoDefaultConstructor3730 {
Expand Down Expand Up @@ -103,103 +92,115 @@ static class Values {

public void testIgnoreBigInteger() throws Exception
{
final String MOCK_MSG = "mock: deliberate failure for parseBigInteger";
mockStatic(NumberInput.class);
when(NumberInput.parseBigInteger(Mockito.anyString()))
.thenThrow(new IllegalStateException(MOCK_MSG));
when(NumberInput.parseBigInteger(Mockito.anyString(), Mockito.anyBoolean()))
.thenThrow(new IllegalStateException(MOCK_MSG));
StringBuilder stringBuilder = new StringBuilder();
for (int i = 0; i < 999; i++) {
stringBuilder.append(7);
}
final String testBigInteger = stringBuilder.toString();
final String json = genJson(testBigInteger);
ExtractFieldsNoDefaultConstructor3730 ef =
MAPPER.readValue(json, ExtractFieldsNoDefaultConstructor3730.class);
assertNotNull(ef);
// Ok but then let's ensure method IS called, if field is actually mapped,
// first to Number
try {
Object ob = STRICT_MAPPER.readValue(json, UnwrappedWithNumber.class);
fail("Should throw exception with mocking: instead got: "+MAPPER.writeValueAsString(ob));
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
try (MockedStatic<NumberInput> mocked = mockStatic(NumberInput.class)) {
// Set up, mock NumberInput.parseBigInteger() to throw exception
final String MOCK_MSG = "mock: deliberate failure for parseBigInteger";
mocked.when(() -> NumberInput.parseBigInteger(Mockito.anyString()))
.thenThrow(new IllegalStateException(MOCK_MSG));
mocked.when(() -> NumberInput.parseBigInteger(Mockito.anyString(), Mockito.anyBoolean()))
.thenThrow(new IllegalStateException(MOCK_MSG));

// Then start testing!
StringBuilder stringBuilder = new StringBuilder();
for (int i = 0; i < 999; i++) {
stringBuilder.append(7);
}
final String testBigInteger = stringBuilder.toString();
final String json = genJson(testBigInteger);
ExtractFieldsNoDefaultConstructor3730 ef =
MAPPER.readValue(json, ExtractFieldsNoDefaultConstructor3730.class);
assertNotNull(ef);
// Ok but then let's ensure method IS called, if field is actually mapped,
// first to Number
try {
Object ob = STRICT_MAPPER.readValue(json, UnwrappedWithNumber.class);
fail("Should throw exception with mocking: instead got: "+MAPPER.writeValueAsString(ob));
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}
}
}

public void testIgnoreFPValuesDefault() throws Exception
{
final String MOCK_MSG = "mock: deliberate failure for parseDouble";
// With default settings we would parse Doubles, so check
mockStatic(NumberInput.class);
when(NumberInput.parseDouble(Mockito.anyString()))
try (MockedStatic<NumberInput> mocked = mockStatic(NumberInput.class)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We are scoping the static mocking within try-with resources

// Set up, mock NumberInput.parseDouble() to throw exception
final String MOCK_MSG = "mock: deliberate failure for parseDouble";
// With default settings we would parse Doubles, so check
mocked.when(() -> NumberInput.parseDouble(Mockito.anyString()))
.thenThrow(new IllegalStateException(MOCK_MSG));
when(NumberInput.parseDouble(Mockito.anyString(), Mockito.anyBoolean()))
mocked.when(() -> NumberInput.parseDouble(Mockito.anyString(), Mockito.anyBoolean()))
.thenThrow(new IllegalStateException(MOCK_MSG));
final String json = genJson("0.25");
ExtractFieldsNoDefaultConstructor3730 ef =
MAPPER.readValue(json, ExtractFieldsNoDefaultConstructor3730.class);
assertNotNull(ef);

// Ok but then let's ensure method IS called, if field is actually mapped
// First, to "Number"
try {
STRICT_MAPPER.readValue(json, UnwrappedWithNumber.class);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}

// And then to "double"
// 01-Feb-2023, tatu: Not quite working, yet:
try {
STRICT_MAPPER.readValue(json, UnwrappedWithDouble.class);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
e.printStackTrace();
verifyMockException(e, MOCK_MSG);
// Then start testing!
final String json = genJson("0.25");
ExtractFieldsNoDefaultConstructor3730 ef =
MAPPER.readValue(json, ExtractFieldsNoDefaultConstructor3730.class);
assertNotNull(ef);

// Ok but then let's ensure method IS called, if field is actually mapped
// First, to "Number"
try {
STRICT_MAPPER.readValue(json, UnwrappedWithNumber.class);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}

// And then to "double"
// 01-Feb-2023, tatu: Not quite working, yet:
try {
STRICT_MAPPER.readValue(json, UnwrappedWithDouble.class);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
e.printStackTrace();
verifyMockException(e, MOCK_MSG);
}
}
}

public void testIgnoreFPValuesBigDecimal() throws Exception
{
final String MOCK_MSG = "mock: deliberate failure for parseBigDecimal";
ObjectReader reader = MAPPER
.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)
.readerFor(ExtractFieldsNoDefaultConstructor3730.class);

// Now should get calls to `parseBigDecimal`... eventually
mockStatic(NumberInput.class);
when(NumberInput.parseBigDecimal(Mockito.anyString()))
try (MockedStatic<NumberInput> mock = mockStatic(NumberInput.class)) {
// Set up, mock NumberInput.parseBigDecimal() to throw exception
// Now should get calls to `parseBigDecimal`... eventually
final String MOCK_MSG = "mock: deliberate failure for parseBigDecimal";
// With default settings we would parse Doubles, so check
mock.when(() -> NumberInput.parseBigDecimal(Mockito.anyString()))
.thenThrow(new IllegalStateException(MOCK_MSG));
when(NumberInput.parseBigDecimal(Mockito.anyString(), Mockito.anyBoolean()))
mock.when(() -> NumberInput.parseBigDecimal(Mockito.anyString(), Mockito.anyBoolean()))
.thenThrow(new IllegalStateException(MOCK_MSG));

final String json = genJson("0.25");
ExtractFieldsNoDefaultConstructor3730 ef =
reader.readValue(genJson(json));
assertNotNull(ef);

// But then ensure we'll fail with unknown (except does it work with unwrapped?)
reader = reader.with(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

// Ok but then let's ensure method IS called, if field is actually mapped
// First to Number
try {
reader.forType(UnwrappedWithNumber.class).readValue(json);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}

// And then to "BigDecimal"
// 01-Feb-2023, tatu: Not quite working, yet:
try {
reader.forType(UnwrappedWithBigDecimal.class).readValue(json);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
// Then start testing!
ObjectReader reader = MAPPER
.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)
.readerFor(ExtractFieldsNoDefaultConstructor3730.class);

final String json = genJson("0.25");
ExtractFieldsNoDefaultConstructor3730 ef =
reader.readValue(genJson(json));
assertNotNull(ef);

// But then ensure we'll fail with unknown (except does it work with unwrapped?)
reader = reader.with(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

// Ok but then let's ensure method IS called, if field is actually mapped
// First to Number
try {
reader.forType(UnwrappedWithNumber.class).readValue(json);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}

// And then to "BigDecimal"
// 01-Feb-2023, tatu: Not quite working, yet:
try {
reader.forType(UnwrappedWithBigDecimal.class).readValue(json);
fail("Should throw exception with mocking!");
} catch (DatabindException e) {
verifyMockException(e, MOCK_MSG);
}
}
}

Expand Down
14 changes: 4 additions & 10 deletions src/test/java/com/fasterxml/jackson/databind/type/TestTypeFactoryWithClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
import static org.mockito.Mockito.*;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import com.fasterxml.jackson.databind.ObjectMapper;

Expand All @@ -15,9 +12,6 @@
import org.junit.Before;
import org.junit.BeforeClass;

@RunWith(PowerMockRunner.class)
@PrepareForTest(TypeFactory.class)

public class TestTypeFactoryWithClassLoader {
@Mock
private TypeModifier typeModifier;
Expand Down Expand Up @@ -54,7 +48,7 @@ public void testUsesCorrectClassLoaderWhenThreadClassLoaderIsNull() throws Class
verify(spySut).classForName(any(String.class), any(Boolean.class), eq(classLoader));
Assert.assertNotNull(clazz);
Assert.assertEquals(classLoader, spySut.getClassLoader());
Assert.assertEquals(typeModifier,spySut._modifiers[0]);
// Assert.assertEquals(typeModifier,spySut._modifiers[0]);
Copy link
Member Author

@JooHyukKim JooHyukKim Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

All assertions such as this one fail, but simply commented out since we are only after classloader. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@JooHyukKim Yes, I think we are ok wrt not checking TypeModifiers: nice to check, not essential.

Assert.assertEquals(null, Thread.currentThread().getContextClassLoader());
}

Expand All @@ -66,14 +60,14 @@ public void testUsesCorrectClassLoaderWhenThreadClassLoaderIsNotNull() throws Cl
verify(spySut).classForName(any(String.class), any(Boolean.class), eq(classLoader));
Assert.assertNotNull(clazz);
Assert.assertEquals(classLoader, spySut.getClassLoader());
Assert.assertEquals(typeModifier,spySut._modifiers[0]);
// Assert.assertEquals(typeModifier,spySut._modifiers[0]);
}

@Test
public void testCallingOnlyWithModifierGivesExpectedResults(){
TypeFactory sut = mapper.getTypeFactory().withModifier(typeModifier);
Assert.assertNull(sut.getClassLoader());
Assert.assertEquals(typeModifier,sut._modifiers[0]);
// Assert.assertEquals(typeModifier,sut._modifiers[0]);
}

@Test
Expand All @@ -87,7 +81,7 @@ public void testCallingOnlyWithClassLoaderGivesExpectedResults(){
public void testDefaultTypeFactoryNotAffectedByWithConstructors() {
TypeFactory sut = mapper.getTypeFactory().withModifier(typeModifier).withClassLoader(classLoader);
Assert.assertEquals(classLoader, sut.getClassLoader());
Assert.assertEquals(typeModifier,sut._modifiers[0]);
// Assert.assertEquals(typeModifier,sut._modifiers[0]);
Assert.assertNull(mapper.getTypeFactory().getClassLoader());
Assert.assertArrayEquals(null,mapper.getTypeFactory()._modifiers);
}
Expand Down