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.

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

Add register and select ArbitraryBuilder by name #1036

Merged
merged 17 commits into from
Oct 4, 2024
Merged

Add register and select ArbitraryBuilder by name #1036

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cf69e46
Add ArbitraryManager for managing builder candidates
YongGoose Aug 22, 2024
49db542
Rename ArbitraryManagerCandidateList to ArbitraryCandidateManager
YongGoose Aug 23, 2024
4122b1e
Add ArbitraryManagerFactory to register ArbitraryBuilder by name
YongGoose Aug 23, 2024
802378a
Refactor to accept parameters instead of using an interface
YongGoose Aug 28, 2024
e98200a
Add test cases for registerArbitraryByName
YongGoose Aug 28, 2024
65baf7d
Rename method and variable
YongGoose Aug 28, 2024
6fb23b6
Add selectName method
YongGoose Aug 30, 2024
9c4f7f8
Update position of selectedArbitraryBuilders addition
YongGoose Aug 30, 2024
83db7cd
Add generate sampleList test
YongGoose Aug 30, 2024
685aa58
Add selectedArbitraryBuilders list to ArbitraryBuilderContext
YongGoose Aug 31, 2024
2b63c65
Rename matcherOperatorMapsByRegisteredName to mapsByRegisteredName
YongGoose Sep 2, 2024
0193617
Declared selectedArbitraryBuilders as final
YongGoose Sep 3, 2024
3deaa89
Fix recursive loop issue by adding base case check
YongGoose Sep 13, 2024
6b1b21a
Refactor Tests
YongGoose Sep 13, 2024
7a30d04
Refactor selectName method
YongGoose Sep 29, 2024
c8936d4
Remove unused variable in DefaultArbitraryBuilder
YongGoose Oct 2, 2024
113f4d2
Rename variable for better readability
YongGoose Oct 4, 2024
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
25 changes: 22 additions & 3 deletions fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
import static java.util.stream.Collectors.toList;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Stream;

Expand Down Expand Up @@ -53,21 +55,25 @@ public final class FixtureMonkey {
private final MonkeyContext monkeyContext;
private final List<MatcherOperator<? extends ArbitraryBuilder<?>>> registeredArbitraryBuilders = new ArrayList<>();
private final MonkeyManipulatorFactory monkeyManipulatorFactory;
private final Map<String, MatcherOperator<? extends ArbitraryBuilder<?>>> namedArbitraryBuilderMap = new HashMap<>();

public FixtureMonkey(
FixtureMonkeyOptions fixtureMonkeyOptions,
ArbitraryTraverser traverser,
ManipulatorOptimizer manipulatorOptimizer,
MonkeyContext monkeyContext,
List<MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>>> registeredArbitraryBuilders,
MonkeyManipulatorFactory monkeyManipulatorFactory
MonkeyManipulatorFactory monkeyManipulatorFactory,
Map<String, MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>>>
matcherOperatorMapsByRegisteredName
) {
this.fixtureMonkeyOptions = fixtureMonkeyOptions;
this.traverser = traverser;
this.manipulatorOptimizer = manipulatorOptimizer;
this.monkeyContext = monkeyContext;
this.monkeyManipulatorFactory = monkeyManipulatorFactory;
initializeRegisteredArbitraryBuilders(registeredArbitraryBuilders);
initializeNamedArbitraryBuilderMap(matcherOperatorMapsByRegisteredName);
}

public static FixtureMonkeyBuilder builder() {
Expand Down Expand Up @@ -111,7 +117,8 @@ public <T> ArbitraryBuilder<T> giveMeBuilder(TypeReference<T> type) {
builderContext.copy(),
registeredArbitraryBuilders,
monkeyContext,
fixtureMonkeyOptions.getInstantiatorProcessor()
fixtureMonkeyOptions.getInstantiatorProcessor(),
namedArbitraryBuilderMap
);
}

Expand All @@ -138,7 +145,8 @@ public <T> ArbitraryBuilder<T> giveMeBuilder(T value) {
context,
registeredArbitraryBuilders,
monkeyContext,
fixtureMonkeyOptions.getInstantiatorProcessor()
fixtureMonkeyOptions.getInstantiatorProcessor(),
namedArbitraryBuilderMap
);
}

Expand Down Expand Up @@ -194,4 +202,15 @@ private void initializeRegisteredArbitraryBuilders(
this.registeredArbitraryBuilders.add(generatedRegisteredArbitraryBuilder.get(i));
}
}

private void initializeNamedArbitraryBuilderMap(
Map<String, MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>>>
matcherOperatorMapsByRegisteredName
) {
matcherOperatorMapsByRegisteredName.forEach((name, matcherOperator) -> {
MatcherOperator<? extends ArbitraryBuilder<?>> newMatcherOperator =
new MatcherOperator<>(matcherOperator.getMatcher(), matcherOperator.getOperator().apply(this));
namedArbitraryBuilderMap.put(name, newMatcherOperator);
});
}
}
22 changes: 21 additions & 1 deletion fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.util.function.UnaryOperator;
Expand Down Expand Up @@ -76,6 +78,8 @@ public final class FixtureMonkeyBuilder {
private ManipulatorOptimizer manipulatorOptimizer = new NoneManipulatorOptimizer();
private MonkeyExpressionFactory monkeyExpressionFactory = new ArbitraryExpressionFactory();
private final MonkeyContextBuilder monkeyContextBuilder = MonkeyContext.builder();
private final Map<String, MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>>>
matcherOperatorMap = new HashMap<>();
private long seed = System.nanoTime();

// The default plugins are listed below.
Expand Down Expand Up @@ -381,6 +385,21 @@ public FixtureMonkeyBuilder registerGroup(ArbitraryBuilderGroup... arbitraryBuil
return this;
}

public FixtureMonkeyBuilder registerArbitraryByName(
String arbitraryName,
Class<?> type,
Function<FixtureMonkey, ? extends ArbitraryBuilder<?>> arbitraryBuilder
) {
if (matcherOperatorMap.containsKey(arbitraryName)) {
throw new IllegalArgumentException("Duplicated ArbitraryBuilder name: " + arbitraryName);
}
MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>> matcherOperator =
MatcherOperator.assignableTypeMatchOperator(type, arbitraryBuilder);

this.matcherOperatorMap.put(arbitraryName, matcherOperator);
return this;
}

public FixtureMonkeyBuilder plugin(Plugin plugin) {
if (plugin instanceof InterfacePlugin) { // TODO: Added for backward compatibility. It will be removed in 1.1.0
this.defaultInterfacePlugin = (InterfacePlugin)plugin;
Expand Down Expand Up @@ -552,7 +571,8 @@ public FixtureMonkey build() {
manipulatorOptimizer,
monkeyContext,
registeredArbitraryBuilders,
monkeyManipulatorFactory
monkeyManipulatorFactory,
matcherOperatorMap
);
}
}
14 changes: 11 additions & 3 deletions ...re-monkey/src/main/java/com/navercorp/fixturemonkey/resolver/DefaultArbitraryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Consumer;
Expand Down Expand Up @@ -85,6 +86,7 @@ public final class DefaultArbitraryBuilder<T> implements ArbitraryBuilder<T>, Ex
private final List<MatcherOperator<? extends ArbitraryBuilder<?>>> registeredArbitraryBuilders;
private final MonkeyContext monkeyContext;
private final InstantiatorProcessor instantiatorProcessor;
private final Map<String, MatcherOperator<? extends ArbitraryBuilder<?>>> namedArbitraryBuilderMap;

public DefaultArbitraryBuilder(
FixtureMonkeyOptions fixtureMonkeyOptions,
Expand All @@ -95,7 +97,8 @@ public DefaultArbitraryBuilder(
ArbitraryBuilderContext context,
List<MatcherOperator<? extends ArbitraryBuilder<?>>> registeredArbitraryBuilders,
MonkeyContext monkeyContext,
InstantiatorProcessor instantiatorProcessor
InstantiatorProcessor instantiatorProcessor,
Map<String, MatcherOperator<? extends ArbitraryBuilder<?>>> ArbitraryBuilderMapsByRegisteredName
) {
this.fixtureMonkeyOptions = fixtureMonkeyOptions;
this.rootProperty = rootProperty;
Expand All @@ -106,6 +109,7 @@ public DefaultArbitraryBuilder(
this.registeredArbitraryBuilders = registeredArbitraryBuilders;
this.monkeyContext = monkeyContext;
this.instantiatorProcessor = instantiatorProcessor;
this.namedArbitraryBuilderMap = ArbitraryBuilderMapsByRegisteredName;
}

@Override
Expand Down Expand Up @@ -178,6 +182,8 @@ public ArbitraryBuilder<T> setLazy(PropertySelector propertySelector, Supplier<?
);
}

// TODO 다음 PR에서 선택 api 구현

@Override
public ArbitraryBuilder<T> setInner(InnerSpec innerSpec) {
ManipulatorSet manipulatorSet = innerSpec.getManipulatorSet(monkeyManipulatorFactory);
Expand Down Expand Up @@ -510,7 +516,8 @@ public ArbitraryBuilder<T> copy() {
context.copy(),
registeredArbitraryBuilders,
monkeyContext,
instantiatorProcessor
instantiatorProcessor,
namedArbitraryBuilderMap
);
}

Expand Down Expand Up @@ -560,7 +567,8 @@ private <R> DefaultArbitraryBuilder<R> generateArbitraryBuilderLazily(LazyArbitr
context,
registeredArbitraryBuilders,
monkeyContext,
instantiatorProcessor
instantiatorProcessor,
namedArbitraryBuilderMap
);
}

Expand Down
49 changes: 49 additions & 0 deletions fixture-monkey/src/test/java/com/navercorp/fixturemonkey/test/FixtureMonkeyOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.assertj.core.api.BDDAssertions.thenThrownBy;

import java.lang.reflect.AnnotatedType;
import java.lang.reflect.Field;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.ArrayList;
Expand Down Expand Up @@ -800,6 +801,54 @@ void registerBuilderGroup() {
then(actual3).isEqualTo(ChildBuilderGroup.FIXED_INT_VALUE);
}

@Property
void registerArbitraryByName() throws NoSuchFieldException, IllegalAccessException {
Copy link
Contributor

Choose a reason for hiding this comment

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

엇 테스트는 reflection으로 내부 상태를 조회해서 검증하는 방향이 아니라
실제 생성한 값이 register한 값인지 확인하는 방향으로 진행하면 좋을 것 같습니다.

내부 상태는 리팩토링하면 언제든지 변경이 될 수 있으므로 사용자와 동일하게 public한 API를 사용해서 테스트하는 게 추후 변경 가능성을 높이는 데 도움이 됩니다.

Copy link
Contributor

@seongahjo seongahjo Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

이해하기 쉽게 PR이 실행 가능한 단위가 되면 좋을 것 같습니다.

설명이 부족했는데요, 실행 가능한 단위는 외부에 공개된 public한 API을 통해 간접적으로 실행 가능하다는 의미이긴 합니다. ex, 구현한 기능이 ArbitraryBuilder#sample을 통해 실행이 되는지

Copy link
Author

Choose a reason for hiding this comment

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

@seongahjo
저도 성아님의 의견에 동의합니다.
하지만 현재는 registeredName API를 통해서 register한 연산을 등록하는 기능만 구현을 한 상태입니다.

그래서 테스트를 하더라도 실제 생성한 값이 register한 값인지 테스트 할 수 없습니다..!

실제 생성한 값과 register한 값을 비교하는 테스트는 select를 구현하는 다음 PR에서 구현을 할 계획이었습니다!
이번 PR에서는 등록을 하는 것이 목적이라 생각해 public한 API를 사용해 테스트를 하는 것은 어느정도 한계가 있을 것 같습니다!

Copy link
Contributor

@seongahjo seongahjo Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

PR을 그렇게 나누신 이유가 있으실까요??
말씀주신 방법대로 나누는 방법으로 하면 PR이 작아지기는 하는데요, 지금 단계에서는 구현이 어떤 방향으로 진행이 될지 알기 어려워서 리뷰하기가 어려운 측면이 있습니다. 자세한 의도는 모르겠지만, 지금 드는 생각으로는 가능하면 테스트를 작성할 수 있는 단위까지 구현하시면 좋지 않을까 생각이 듭니다.

리팩토링같은 엄청 큰 작업이 아니라 이 PR에서 아래 작업까지 구현하셔도 괜찮지 않을까 싶습니다.

실제 생성한 값과 register한 값을 비교하는 테스트는 select를 구현하는 다음 PR

Copy link
Author

Choose a reason for hiding this comment

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

성아님과 이메일을 주고 받을 때 단계 별로 진행해보는 것이 좋을 것 같다고 하셔서 여러 개의 PR로 작업한다고 이해를 했습니다..!😅

리팩토링같은 엄청 큰 작업이 아니라 이 PR에서 아래 작업까지 구현하셔도 괜찮지 않을까 싶습니다.

넵! 그러면 테스트를 작성할 수 있는 단위까지 구현한 뒤 테스트를 수정하도록 하겠습니다!

Copy link
Contributor

@seongahjo seongahjo Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

넵, 여러 개의 PR을 말씀드렸던 것은 기능 구현을 여러 단계로 구현하는 방향을 말씀드린 거긴 합니다.

ex. name을 키로 등록하는 register할 수 있게 구현 -> (name, 타입)을 키로 register할 수 있게 구현

FixtureMonkey sut = FixtureMonkey.builder()
.registerArbitraryByName(
"test",
String.class,
monkey -> monkey.giveMeBuilder("test")
)
.registerArbitraryByName(
"test2",
String.class,
monkey -> monkey.giveMeBuilder("test2")
)
.registerArbitraryByName(
"test3",
String.class,
monkey -> monkey.giveMeBuilder("test3")
)
.build();

Field matcherOperatorMapField = sut.getClass().getDeclaredField("namedArbitraryBuilderMap");
matcherOperatorMapField.setAccessible(true);
Map<String, MatcherOperator<?>> matcherOperatorMap =
(Map<String, MatcherOperator<?>>)matcherOperatorMapField.get(sut);

then(matcherOperatorMap)
.hasSize(3)
.containsKeys("test", "test2", "test3");
}

@Property
void registerArbitraryByNameWithSameNameThrows() {
thenThrownBy(() -> FixtureMonkey.builder()
.registerArbitraryByName(
"test",
String.class,
monkey -> monkey.giveMeBuilder("test")
)
.registerArbitraryByName(
"test",
String.class,
monkey -> monkey.giveMeBuilder("test2")
)
.build()
).isExactlyInstanceOf(IllegalArgumentException.class)
.hasMessage("Duplicated ArbitraryBuilder name: test");
}

@Property
void registerSameInstancesTwiceWorksLast() {
FixtureMonkey sut = FixtureMonkey.builder()
Expand Down