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.

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

Refactor : Adjust resolveFieldName Method Logic to Correctly Handle Primitive boolean Field Names #1096

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@ public final class JavaGetterPropertyFieldNameResolver {

@Nullable
public String resolveFieldName(Class<?> targetClass, String methodName) {
if (hasPrefix(GET_PREFIX, methodName)) {

if (isValidField(targetClass, methodName)) {
Copy link
Contributor

@seongahjo seongahjo Nov 20, 2024

Choose a reason for hiding this comment

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

하위호환을 보장하고 안전하게 변경됨을 보장하기 위해 다음 테스트들이 추가되면 좋을 것 같습니다.

  1. is가 이름의 prefix로 붙은 boolean이 아닌 필드를 set
  2. is가 이름의 prefix로 붙지않은 boolean의 필드를 set
  3. is가 이름의 prefix로 붙은 boolean인 필드를 set
  4. is가 이름의 prefix로 붙지않은 boolean이 아닌 필드를 set

Copy link
Author

@rawfishthelgh rawfishthelgh Nov 25, 2024

Choose a reason for hiding this comment

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

테스트 추가했습니다!

  1. is가 이름의 prefix로 붙은 boolean이 아닌 필드를 set
  2. is가 이름의 prefix로 붙지않은 boolean의 필드를 set
  3. is가 이름의 prefix로 붙은 boolean인 필드를 set
  4. is가 이름의 prefix로 붙지않은 boolean이 아닌 필드를 set

추가로 Boolean 필드가 Wrapper Type일 경우와 Primitive Type일 경우의 테스트도 작성했습니다.

// class could be using property-style getters (e.g. java record)
return methodName;
} else if (hasPrefix(GET_PREFIX, methodName)) {
return stripPrefixPropertyName(targetClass, methodName, GET_PREFIX.length());
} else if (hasPrefix(IS_PREFIX, methodName)) {
return stripPrefixPropertyName(targetClass, methodName, IS_PREFIX.length());
} else if (isValidField(targetClass, methodName)) {
// class could be using property-style getters (e.g. java record)
return methodName;
}

return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package com.navercorp.fixturemonkey.tests.java;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.lang.reflect.Method;

import org.junit.jupiter.api.Test;

import lombok.Getter;

import com.navercorp.fixturemonkey.api.expression.JavaGetterPropertyFieldNameResolver;

public class JavaGetterPropertyFieldNameResolverTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

License를 추가하고, public을 제거하면 좋을 것 같습니다.


private final JavaGetterPropertyFieldNameResolver resolver = new JavaGetterPropertyFieldNameResolver();
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 대상을 식별하기 편하게 하기 위해 sut라고 이름을 짓는 게 좋은 것 같습니다.


@Test
void testNonBooleanFieldWithIsPrefix() {
assertDoesNotThrow(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 테스트들을 보고 컨벤션을 맞춰주시면 좋을 것 같습니다.

Method method = TestClass.class.getDeclaredMethod("getIsStatus");
String fieldName = resolver.resolveFieldName(TestClass.class, method.getName());
assertEquals("isStatus", fieldName,
"Should strip 'get' from a non-boolean getter with the 'is' prefix.");
}, "Method 'getIsStatus' should exist and not throw an exception.");
}

@Test
void testPrimitiveTypeBooleanFieldWithIsPrefix() {
assertDoesNotThrow(() -> {
Method method = TestClass.class.getDeclaredMethod("isActive");
String fieldName = resolver.resolveFieldName(TestClass.class, method.getName());
assertEquals("isActive", fieldName,
"Should not strip the 'is' prefix from a getter for a primitive boolean field.");
}, "Method 'isActive' should exist and not throw an exception.");
}

@Test
void testBooleanFieldWithoutIsPrefix() {
assertDoesNotThrow(() -> {
Method method = TestClass.class.getDeclaredMethod("isEnabled");
String fieldName = resolver.resolveFieldName(TestClass.class, method.getName());
assertEquals(
"enabled", fieldName,
"Should strip the 'is' prefix from "
+ "a getter for a boolean field without the 'is' prefix in the field name.");
}, "Method 'isEnabled' should exist and not throw an exception.");
}

@Test
void testNonBooleanFieldWithoutIsPrefix() {
assertDoesNotThrow(() -> {
Method method = TestClass.class.getDeclaredMethod("getName");
String fieldName = resolver.resolveFieldName(TestClass.class, method.getName());
assertEquals("name", fieldName,
"Should strip 'get' from a getter for a non-boolean field without the 'is' prefix.");
}, "Method 'getName' should exist and not throw an exception.");
}

@Test
void testWrapperTypeBooleanFieldWithIsPrefix() {
assertDoesNotThrow(() -> {
Method method = TestClass.class.getDeclaredMethod("getIsDeleted");
String fieldName = resolver.resolveFieldName(TestClass.class, method.getName());
assertEquals("isDeleted", fieldName,
"Should strip 'get' from a getter for a wrapper type Boolean field with the 'is' prefix.");
}, "Method 'getIsDeleted' should exist and not throw an exception.");
}

@Getter
private static class TestClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

이름이 더 구체적이면 좋을 것 같습니다.

JavaGetterObject 라고 바꾸면 javaGetter를 테스트하기 위한 용도로 이해할 수 있을 것 같습니다.

private String isStatus;
private boolean isActive;
private boolean enabled;
private String name;
private Boolean isDeleted;
}
}
Loading