-
Notifications
You must be signed in to change notification settings - Fork 84
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 unique efficiently for set, map #465
Conversation
fixture-monkey-api/src/main/java/com/navercorp/fixturemonkey/api/unique/UniqueCache.java
Outdated
Show resolved
Hide resolved
if (isFixed()) { | ||
return super.generator(genSize); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
값이 고정된 경우에는 필터링에서 제외합니다.
|
||
@SuppressWarnings("rawtypes") | ||
public ArbitraryGeneratorContext( | ||
ArbitraryProperty property, | ||
List<ArbitraryProperty> children, | ||
@Nullable ArbitraryGeneratorContext ownerContext, | ||
BiFunction<ArbitraryGeneratorContext, ArbitraryProperty, Arbitrary<?>> resolveArbitrary, | ||
List<MatcherOperator<? extends FixtureCustomizer>> fixtureCustomizers | ||
List<MatcherOperator<? extends FixtureCustomizer>> fixtureCustomizers, | ||
Map<Property, Set<Object>> uniqueSetsByProperty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property를 키로 설정하면 제네릭이 다른 경우에는 다른 타입으로 인식합니다.
지금은 맵 클래스, 셋 클래스를 키로 설정하므로 중첩된 경우에 문제가 되지 않습니다.
일반적인 클래스를 키로 설정할 필요가 있다면 변경할 예정입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전에 scope 별로 상태를 관리할 수 있는 Context 를 만들어서 전달하면 좋겠다는 이야기를 했었는데요
지금 그 껍데기 클래스를 만들고 거기에 uniqueSetsByProperty
를 넣으면 좋을거 같습니다.
이후에 관련된 변경이 있을 때 이 생성자가 하위호환을 깨게될거 같아 미리 껍데기 클래스를 생성자가 가지도록 만들면 이후 하위호환을 깨지 않고 변경하기 용이할거 같습니다.
ExpressionGeneratorTest 의 sizeMapWithExpressionGenerator에서도 Todo 주석이랑 tries 지워주셔도 될 것 같습니다! |
void sampleUniqueSet() { | ||
// when | ||
Set<String> actual = SUT.giveMeBuilder(new TypeReference<Set<String>>() { | ||
}) | ||
.size("$", 200) | ||
.sample(); | ||
|
||
then(actual).hasSize(200); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만들 수 있는 String에 제약이 걸려 있어서 설정한 size만큼 Set의 원소를 만들지 못하면 filter 에러가 나는 것이 맞을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 맞아욤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자가 문제 원인을 더 파악하기 쉽도록 예외처리(예외 메세지)를 추가하는 것은 어떨까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외를 발생시키는 주제가 조합한 Arbitrary이기 때문에 예외처리 작업이 매우 커질 것 같네요.
이건 조금 더 고민해보겠습니다.
|
||
@SuppressWarnings("NullableProblems") | ||
@API(since = "0.4.3", status = Status.EXPERIMENTAL) | ||
public final class UniqueArbitraryFilter<T> extends ArbitraryDelegator<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스가 Arbitrary 인터페이스를 구현한 것 같은데 혹시 이름에 Filter를 넣은 이유가 있으실까요?
이름에 filter가 들어가서 개인적으로 filterPredicate이랑 헷갈리는 것 같습니다! (성아님께서 괜찮으신 것 같으면 이대로 가도 상관없긴 합니다)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary 인터페이스를 구현했지만 실제 필요한건 RandomGenerator
구현 이긴합니다.
FilteredGenerator
와 동일한 동작을 해야하지만 Arbitrary 인터페이스로 제공하기 위해 ArbitraryDelegator
를 확장해서 만들었습니다.
역할이 계속 바뀌다보니 이름이 이상해진 것 같은데 FilteredGenerator
와 유사하게 FilteredArbitrary
로 변경하는 게 좋겠군요.
jqwik 구현체로 헷갈릴 수 있으므로 FilteredMonkeyArbitrary
로 변경하겠습니다.
@Override | ||
public boolean equals(Object obj) { | ||
if (this == obj) { | ||
return true; | ||
} | ||
if (obj == null || getClass() != obj.getClass()) { | ||
return false; | ||
} | ||
RootProperty that = (RootProperty)obj; | ||
return annotatedType.getType().equals(that.annotatedType.getType()); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(annotatedType.getType()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RootProperty는 동일성으로만 비교합니다. 따라서 캐싱 대상에서 제외됩니다.
RootProperty 자식 Property들까지만 캐싱을 적용합니다.
@@ -25,13 +25,19 @@ | |||
|
|||
import com.navercorp.fixturemonkey.api.collection.LruCache; | |||
import com.navercorp.fixturemonkey.api.property.Property; | |||
import com.navercorp.fixturemonkey.api.property.RootProperty; | |||
|
|||
@API(since = "0.4.0", status = Status.EXPERIMENTAL) | |||
public final class MonkeyContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MonkeyContext는 LabMonkey scope, MonkeyGeneratorContext에서는 ArbitraryGeneratorContext scope입니다.
Set, Map을 생성할 때 항상 유니크하게 생성하도록 수정합니다.
수정하니
filter
메서드로 인해 실패하는 경우가 많이 줄었습니다.resolves #420