Implement Random Selection of Register Operations When Not Using selectName #1108

YongGoose · 2024-11-29T08:08:50Z

Summary

resolved #1081

Randomized Operation Selection:

Before: Always applied the first registered operation in cases of multiple registrations.
After: Randomly selects an operation from those registered via the registeredName method.

Open Question

In the registeredName, if there are register operations with assigned priorities and others without, how should the operations without assigned priorities be handled?
Is the priority field allowed to be null?

How Has This Been Tested?

existing tests

Is the Document updated?

Later

YongGoose · 2024-11-29T08:23:46Z

@seongahjo

Random selection의 경우 의도한대로 등록한 순서가 아닌, 무작위 순서로 구현을 하였습니다.
아래의 테스트를 실행한 결과 100번 중 34번 성공함을 확인했습니다.

@RepeatedTest(100)
void registeredNameWithNoSelectName() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registeredName(
			"test",
			String.class,
			monkey -> monkey.giveMeBuilder("test")
		)
		.registeredName(
			"test2",
			String.class,
			monkey -> monkey.giveMeBuilder("test2")
		)
		.registeredName(
			"test3",
			String.class,
			monkey -> monkey.giveMeBuilder("test3")
		)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

우선 순위의 경우 Open Question에도 작성하긴 했지만, 우선순위를 배정한 register 연산과 배정하지 않은 register 연산이 있을 때 어떻게 처리를 해야 할 지 정해야 할 것 같습니다.

아래의 두 가지 방법이 떠오르긴 하는데, 한 번 의견 부탁드립니다!

우선순위를 배정하지 않은 것은 임의의 우선순위를 배정한다.
등록한 순서대로 우선 순위를 부여한다. (오름차순)

또한 우선순위를 null로 설정할 수 있는지도 결정해야 할 것 같습니다. ex. (@nullable)
만약 무조건 우선순위를 부여해야 한다면 별도의 API로 분리 시키는 것도 괜찮을 것 같습니다.

seongahjo · 2024-12-01T07:30:57Z

아래의 두 가지 방법이 떠오르긴 하는데, 한 번 의견 부탁드립니다!

넵, 말씀주신 방법이 좋아보입니다. 우선순위가 있는 register 연산이 있다면 우선순위가 있는 연산끼리 경쟁하고, 등록한 register 연산이 모두 우선순위가 없다면 모두 랜덤하게 처리하는 방향이 좋아보입니다.

이 정책은 정리해서 문서나 JavaDoc으로 남겨두면 좋을 것 같습니다.

만약 무조건 우선순위를 부여해야 한다면 별도의 API로 분리 시키는 것도 괜찮을 것 같습니다.

두가지 옵션 registerGroup와 register가 일관적인 API로 우선순위를 입력받는 게 좋을 것 같은데, 이 부분을 고민해보시면 좋을 것 같습니다.

seongahjo · 2024-12-01T07:32:22Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkey.java

@@ -204,13 +206,18 @@ private void initializeRegisteredArbitraryBuilders(
 	private void initializeNamedArbitraryBuilderMap(


이러면 registerName이 아닌 register에는 적용이 안되지 않나요??

register 연산에 모두 적용해야 하는데 registeredName을 통해 등록하는 register 연산만 적용하는 것으로 착각했네요.😅
수정하겠습니다.

해당 commit에서 작업했습니다. b2abc78

검토 부탁드립니다 :)

YongGoose · 2024-12-01T07:43:14Z

이 정책은 정리해서 문서나 JavaDoc으로 남겨두면 좋을 것 같습니다.

넵!
해당 메서드에 javadoc으로 자세히 서술하겠습니다.

두가지 옵션 registerGroup와 register가 일관적인 API로 우선순위를 입력받는 게 좋을 것 같은데, 이 부분을 고민해보시면 좋을 것 같습니다.

register 연산을 등록하는 API가 일관적인 방법으로 우선순위를 입력받는 것이 좋다고 이해하면 될까요?

seongahjo · 2024-12-01T08:32:44Z

register 연산을 등록하는 API가 일관적인 방법으로 우선순위를 입력받는 것이 좋다고 이해하면 될까요?

넵, registerGroup은 reflection 혹은 ArbitraryBuilderGroup으로 입력받을 수 있고,
register는 타입 혹은 Matcher와 함께 연산을 입력할텐데요, 어떤 방식으로 우선순위를 지정해야 이 모든 방법에서 일관적으로 제공할 수 있을지 고민이 필요할 것 같습니다.

예를 들어, reflection으로 등록하는 registerGroup에서는 등록할 때 클래스에 어노테이션으로 마킹해서 우선순위를 지정할 수 있을텐데요, 다른 입력지점에서는 어노테이션으로 제공하기가 어렵습니다.

입력 방법이 일관적이지 않더라도, 사람들이 자주 쓰는 패턴이여서 인지적 비용이 적다면 일관적이지 않아도 괜찮을 것 같긴 합니다.

Signed-off-by: yongjunhong <kevin0928@naver.com>

YongGoose · 2024-12-05T05:27:15Z

우선순위 기능을 해당 커밋에서 작업했습니다. c143264

간략하게 설명하면, register, registeredName의 경우 우선순위를 받는 API를 하나 더 생성해서 진행했습니다.

우선순위를 부여하지 않았을 땐 priority를 Integer.MAX_VALUE로 설정해 Last Place에 배치했습니다. (기존의 API)
아래의 코드와 같이 파라미터로 정수 값을 받도록 구현했습니다.

registeredName도 파라미터에서 priority 값을 받도록 구현했습니다.

아직 문서 작업은 하지 않은 상태입니다. code 관련 작업이 끝난 뒤, javadoc 작업 하겠습니다🙂

public FixtureMonkeyBuilder register(
	Class<?> type,
	Function<FixtureMonkey, ? extends ArbitraryBuilder<?>> registeredArbitraryBuilder,
	Integer priority
) {
	return this.register(MatcherOperator.assignableTypeMatchOperator(type, registeredArbitraryBuilder), priority);
}

이때 우선순위는 0보다 커야합니다.

registerGroup의 경우 @Order 어노테이션을 통해 우선순위를 부여하는 방법과, 낱개로 우선순위를 부여하는 방법으로 구현했습니다.

Spring-Boot, JUnit 사용자로 하여끔 친숙하게 사용할 수 있도록 Order이라는 이름을 채택했습니다.

낱개로 등록하는 방법입니다.

public FixtureMonkeyBuilder registerGroup(
	ArbitraryBuilderGroup arbitraryBuilderGroup,
	Integer priority
) {
	List<ArbitraryBuilderCandidate<?>> candidates = arbitraryBuilderGroup.generateCandidateList()
		.getCandidates();

	for (ArbitraryBuilderCandidate<?> candidate : candidates) {
		this.register(
			candidate.getClassType(),
			candidate.getArbitraryBuilderRegisterer(),
			priority
		);
	}
	return this;
}

@Order 어노테이션을 통해 등록하는 방법입니다.

if (arbitraryBuilderGroup.isAnnotationPresent(Order.class)) {
	Order order = arbitraryBuilderGroup.getAnnotation(Order.class);
	this.register(actualType, registerArbitraryBuilder, order.priority());
}
this.register(actualType, registerArbitraryBuilder);

아래의 테스트 코드를 실행한 결과 예상한대로 작동함을 확인했습니다.
또한, registeredName과 register을 같이 사용하는 경우에서도 예상한대로 작동함을 확인했습니다.

register(우선순위 1), registeredName(우선순위 2) -> register 적용

@Property
void registerWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.register(String.class, monkey -> monkey.giveMeBuilder("test"), 1)
		.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

@Property
void registeredNameWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registeredName(
			"test",
			String.class,
			monkey -> monkey.giveMeBuilder("test"),
			1
		)
		.registeredName(
			"test2",
			String.class,
			monkey -> monkey.giveMeBuilder("test2"),
			2
		)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

@Property
void registerGroupWithPriorityAnnotation() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registerGroup(RegisterGroup.class, RegisterGroupWithPriority.class)
		.build();

	String actual = sut.giveMeOne(SimpleObject.class)
		.getStr();
	List<String> actual2 = sut.giveMeOne(new TypeReference<List<String>>() {
	});
	ConcreteIntValue actual3 = sut.giveMeOne(ConcreteIntValue.class);

	then(actual).hasSizeBetween(4, 6);
	then(actual2).hasSizeGreaterThan(4);
	then(actual3).isEqualTo(RegisterGroup.FIXED_INT_VALUE);
}

@Property
void registerGroupWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registerGroup(new SiblingBuilderGroup(), 1)
		.registerGroup(new ChildBuilderGroup(), 2)
		.build();

	String actual = sut.giveMeOne(SimpleObject.class)
		.getStr();
	List<String> actual2 = sut.giveMeOne(new TypeReference<List<String>>() {
	});
	ConcreteIntValue actual3 = sut.giveMeOne(ConcreteIntValue.class);

	then(actual).hasSizeBetween(4, 6);
	then(actual2).hasSizeGreaterThan(3);
	then(actual3).isEqualTo(ChildBuilderGroup.FIXED_INT_VALUE);
}

@Property
void registerWithPriorityLessThenZero() {
	thenThrownBy(() -> FixtureMonkey.builder()
		.register(String.class, monkey -> monkey.giveMeBuilder("test"), -1)
		.build()
	).isExactlyInstanceOf(IllegalArgumentException.class)
		.hasMessage("Priority must be greater than or equal to 0");
}

seongahjo · 2024-12-06T04:13:43Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/MatcherInfo.java

+import com.navercorp.fixturemonkey.FixtureMonkey;
+import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;
+
+public interface MatcherInfo {


이게 필요한 이유는 뭔가요??

다음과 같이 처리를 설계하였으며, 이를 위해 인터페이스를 통해 접근하도록 구성했습니다.

fixture-monkey/fixture-monkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/MatcherMetadata.java

Line 21 in e508ba8

public interface MatcherMetadata {

하지만 MatcherMetadata와 다르게 이번 경우에는 내부 구현을 감출 필요가 없다는 생각이 드네요.

따라서 인터페이스를 삭제하고 구현 클래스에 바로 접근을 하도록 수정하려고 하는데 어떻게 생각하시나요?

새로운 인터페이스를 추가하는 이유가 뭔가요?? 새로운 관심사라고 보시는건가요??

관심사를 프로그램의 특정 기능이나 책임을 나타내는 독립적인 부분으로 정의한다면, 우선순위가 적용된 register 연산을 하나의 관심사로 간주할 수 있다고 생각합니다.

우선 제가 생각하는 관심사 분리의 장점은 재사용성과 유지보수성 향상입니다.

우선, 우선순위가 적용된 register 연산은 재사용이 가능합니다.
예를 들어, 다른 모듈에서 유사한 우선순위 기반 등록 메커니즘이 필요할 때, 이 로직을 그대로 활용할 수 있습니다.

만약 다른 case에도 사용을 하려고 한다면 Generic를 사용해 우선순위가 적용된 Object를 받는 것이 좋을 것 같습니다.

또한, 우선순위가 적용된 register 연산에서 우선순위 입력 정책을 변경하려고 할 때 시스템의 다른 부분에 영향을 주지 않고 독립적으로 변경할 수 있습니다.
예를 들어, 0 미만의 숫자를 입력하면 예외를 발생시킨다는 정책을 -100으로 수정할 때, 해당 로직만 수정하면 되므로 다른 코드에 영향을 주지 않습니다.

하지만 MatcherMetadata와 다르게 이번 경우에는 내부 구현을 감출 필요가 없다는 생각이 드네요.

해당 코멘트를 작성한 이유는, MatcherMetadata와 달리 MatcherInfo는 단일 구현체만 존재할 가능성이 높아 보였기 때문입니다. 따라서 불필요한 추상화 계층을 제거하는 것이 더 적합하다고 판단했습니다.

@seongahjo 어떻게 생각하시나요?

인터페이스를 분리하면 구현에 따라서 register 연산과 우선순위를 가지는 register 연산이 다르게 처리되는 문제가 생길수도 있을 것 같습니다. 실행흐름이 두 개로 분리되는데 오히려 유지보수가 어려워지는 게 아닐까요? 코드가 기존 흐름대로 실행되지 않고 계속 새로운 필드를 추가하고 독립적인 실행흐름을 만드는 코드는 피하시는 게 좋아보입니다. 개인적인 생각에는, 기존 흐름을 유지하면서 새로운 기능을 제공할 수 있을지 고민이 필요해보입니다.

인터페이스를 새로 추가하는 건 큰 비용이므로 정말 불가피하지 않는 생각하시지 않는 게 좋아보입니다. 인터페이스를 나누는 기준에 대해서 다시 생각해보시는 게 좋을 것 같습니다.

예를 들어, 다른 모듈에서 유사한 우선순위 기반 등록 메커니즘이 필요할 때, 이 로직을 그대로 활용할 수 있습니다.

재사용이 될지도 모르는 코드를 추상화하는 시도는 위험한 것 같은데요, 최소한 3번 이상 쓰여야 추상화를 할지 결정하는 방향이 좋은 것 같습니다. 특히, 고려해야할 요소가 많고 유즈 케이스가 명백하지 않은 경우인 것 같아서 지금 단계에서 추상화는 위험해보입니다.
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

저는 MatcherInfo라는 이름에서 우선순위가 적용된 register 연산 이라는 정보를 유추하기가 어려운 것 같은데요, 이건 어떻게 알 수 있을까요??

실행흐름이 두 개로 분리되는데 오히려 유지보수가 어려워지는 게 아닐까요?

사용 흐름을 고려하지 않았네요,, 좋은 지적 감사합니다.

재사용이 될지도 모르는 코드를 추상화하는 시도는 위험한 것 같은데요, 최소한 3번 이상 쓰여야 추상화를 할지 결정하는 방향이 좋은 것 같습니다.

동의합니다. 재사용 여부가 불확실한 코드는 추상화하기에 위험할 것 같네요,,
건설적인 피드백 감사합니다.

앞으로 리팩토링 할 때 꼭 참고하겠습니다!🙂

저는 MatcherInfo라는 이름에서 우선순위가 적용된 register 연산 이라는 정보를 유추하기가 어려운 것 같은데요, 이건 어떻게 알 수 있을까요??

해당 네이밍은 성아님과 함께 대화를 하며 수정하려고 했습니다!
코드 설계 만큼 네이밍 어렵네요,,🥲

인터페이스를 삭제하고 클래스를 참조하도록 변경하겠습니다.

Class name도 한 번 더 고민해보겠습니다..

덕분에 인사이트가 넓어진 것 같습니다. 감사합니다 :)

seongahjo · 2024-12-06T04:14:12Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/MatcherInfo.java

+import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;
+
+public interface MatcherInfo {
+	Integer getPriority();


왜 primitive 타입이 아니라 wrapper 타입을 반환하는 걸까요??

처음에 null을 고려하고 설계를 해서 wrapper 클래스로 작성을 했습니다.
하지만, null을 허용하지 않기 때문에 primitive 타입으로 변경하겠습니다. 😅

아래의 코멘트에서 방향이 결정된 뒤 수정하겠습니다.

Implement Random Selection of Register Operations When Not Using selectName #1108 (comment)

Signed-off-by: yongjunhong <kevin0928@naver.com>

seongahjo · 2024-12-07T06:00:05Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkey.java

+	private final Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>>
+		priorityGroupedMatchers = new HashMap<>();


priorityGroupedMatchers 와 registeredArbitraryBuilders 중 하나만 남기는 방향이 좋을 것 같습니다.

priorityGroupedMatchers의 이름 안에 register를 포함해야할 것 같습니다.

seongahjo · 2024-12-07T06:02:01Z

...ure-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/PriorityMatcherOperator.java

+import com.navercorp.fixturemonkey.FixtureMonkey;
+import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;
+
+public final class PriorityMatcherOperator {


MatcherOperator는 단순히 Matcher와 Operator를 가지고 Operator 적용 여부(match)는 외부에서 판단합니다.

적용 여부를 내부에서 판단하는 새로운 인터페이스를 설계하면 PriorityMatcherOperator 도 그 인터페이스의 구현체 중 하나로 내부 구조 노출없이 일관성있게 제공할 수 있지 않을까 싶습니다.

궁금한 점이 있어 코멘트 남깁니다!

현재 PriorityMatcherOperator의 경우 아래의 경우로 사용되는 것으로 알고 있습니다.

Collectors.mapping( it -> new MatcherOperator<>( it.getMatcherOperator(), it.getMatcherOperator().getOperator().apply(this) ),

register 연산을 입력 받을 때 파라미터로 입력 받은 Function 함수형 인터페이스를 가지고 있다가 apply를 통해 ArbitrartyBuilder<?>을 생성하는 것으로 알고 있습니다.

그런 뒤, registeredArbitraryBuilders에 추가를 하고 giveMeBuilder에서 match를 통해 적용 여부를 판단하는 것으로 확인이 됩니다.

이렇게만 본다면 단순히 MatcherOperator을 우선순위와 함께 정보를 가지고 있다가 초기화를 해주는 것으로 보이는데요, 아래와 같이 PriorityMatcherOperator가 내부에서 match를 해야 하는 case가 있는지 궁금합니다.

적용 여부를 내부에서 판단하는 새로운 인터페이스를 설계하면 PriorityMatcherOperator 도 그 인터페이스의 구현체 중 하나로 내부 구조 노출없이 일관성있게 제공할 수 있지 않을까 싶습니다.

질문을 잘 이해 못했습니다.
초기화는 match랑 상관이 없는 로직인데 어떤 연관성이 있다고 생각하시는 걸까요?

제가 드린 의견은 우선순위 정보를 외부에 노출할 필요가 없게 만드는 게 좋다는 의견이라고 보시면 될 것 같습니다.
우선순위 정보는 구현에 강하게 의존한 정보라고 보고 있습니다.

아아 확인했습니다.

제가 성아님의 코멘트를 잘못 이해한 것 같네요,,😅
작업 후 리뷰 요청하겠습니다!

제가 드린 의견은 우선순위 정보를 외부에 노출할 필요가 없게 만드는 게 좋다는 의견이라고 보시면 될 것 같습니다.

해당 방법으로 설계를 진행하고 있는데 어려움을 겪고 있습니다.
혹시 성아님께서 생각하신 구체적인 구현 방안이 있다면 간단히 설명해주실 수 있을까요?

제가 어려움을 겪는 부분을 자세히 설명드리자면, 아래의 코드에서 getPriority 메서드를 통해 우선순위 정보를 노출하지 않고 진행하는 것에 어려움을 겪고 있습니다.

현재는 우선순위 : List<연산>의 Key-Value 형태로 저장을 한 뒤, 각 Key에 해당하는 Value를 섞는 방법으로 진행을 하고있어, Key에 해당하는 우선순위 정보를 가져와야 하는 상황입니다.

private Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>> groupMatchersByPriority( List<PriorityMatcherOperator> registeredArbitraryBuildersWithPriority ) { return new HashMap<>(registeredArbitraryBuildersWithPriority .stream() .collect(Collectors.groupingBy( PriorityMatcherOperator::getPriority, Collectors.mapping( it -> new MatcherOperator<>( it.getMatcherOperator(), it.getMatcherOperator().getOperator().apply(this) ), Collectors.toList() ) )) ); } private void groupMatchersByPriorityFromNamedMap( Map<String, PriorityMatcherOperator> registeredPriorityMatchersByName, Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>> registeredGroupMatchersListByPriority ) { registeredGroupMatchersListByPriority.putAll( registeredPriorityMatchersByName.entrySet().stream() .collect(Collectors.groupingBy( entry -> entry.getValue().getPriority(), Collectors.mapping( entry -> new MatcherOperator<>( new NamedMatcher(entry.getValue().getMatcherOperator().getMatcher(), entry.getKey()), entry.getValue().getMatcherOperator().getOperator().apply(this) ), Collectors.toList() ) )) ); }

@YongGoose

저는 무작위로 섞는 연산이나, 초기화된 인스턴스를 정렬 하는 것을 생각 하고 있습니다.

넵, 저도 말씀주신 단계로 진행하는 게 좋을 것 같습니다!
위에 두 가지 경우가 각각 trade-off가 있을텐데요, 두 경우 중 어떤 경우가 기본 옵션으로 제공하는 게 좋을지 고민해보시고 진행하시면 좋을 것 같다는 생각이 듭니다.

한 가지 더 공유드리고 싶은 점은, 시드 값이 동일하면 정렬 결과가 항상 같은지 (결정적인지) 확인하는 과정도 필요할 것 같습니다.

@seongahjo

해당 커밋에서 작업 했습니다! d00d7a7

넵, 말씀주신 방법이 좋아보입니다. 우선순위가 있는 register 연산이 있다면 우선순위가 있는 연산끼리 경쟁하고, 등록한 register 연산이 모두 우선순위가 없다면 모두 랜덤하게 처리하는 방향이 좋아보입니다.

위 코멘트와 같이 구현을 했습니다.

저는 연산을 초기화 하는 로직을 총 2가지로 판단을 했습니다.

자식 노드가 있는 경우 (List<ObjectNode> objectNodes = nodeByType.getValue(); 리스트의 크기가 1 이상인 경우)

자식 노드가 없는 경우

자식 노드가 있는 경우, MonkeyManipulatorFactory에서 자식 노드를 초기화 할 때마다 우선순위를 통해 정렬을 수행했습니다.
자식 노드가 없는 경우, FixtureMonkey의 giveMeBuilder에서 정렬을 수행했습니다.

불필요한 cost를 줄이기 위해 match 연산을 통해 초기화 할 수 있는 연산에만 정렬을 수행하려 노력했습니다,,

test를 수행한 결과 모두 통과 하는 것을 확인했습니다.

한 가지 더 공유드리고 싶은 점은, 시드 값이 동일하면 정렬 결과가 항상 같은지 (결정적인지) 확인하는 과정도 필요할 것 같습니다.

정렬 관련 논의가 끝난 뒤, 문서화와 함께 테스트도 함께 작성하겠습니다 :)

동일한 정렬 로직이 두 군데에 적용됨으로 인해 중복 코드가 발생하였습니다.
이를 줄이기 위해 한 번 적용함으로 인해 자식 노드가 있는 경우와 없는 경우에도 모두 정렬을 수행할 수 있는 지점을 찾아보았으나 찾지 못 했습니다..

혹시 성아님이 의도하신 정렬과 제가 적용한 부분에 다른 점이 있다면 말씀 부탁드립니다!🙂

감사합니다.

@seongahjo

안녕하세요 성아님!
혹시 시간 가능하실 때 제 코멘트에 대해 피드백 부탁드려도 될까요?

많은 Fixture Monkey 사용자분들께서 사용하실 수 있게 selectName 기능 및 register 연산 랜덤 적용 기능을 빠르게 머지 하고 싶습니다! :) 🙂

항상 감사합니다!

@YongGoose
죄송합니다; 제가 다른 업무를 하다가 리뷰하는 걸 놓쳤네요.

이제 크게 변경은 필요없을 것 같고 검증이 필요한 테스트들만 더 추가하면 좋을 것 같습니다.

수고하셨습니다!

@seongahjo

아닙니다ㅎㅎ

성아님 덕분에 많이 발전할 수 있었습니다.
확인해주셔서 감사합니다 :)

퇴근한 뒤, 코멘트 달아주신 내용 작업해보려 합니다!

항상 감사합니다. 요즘 날씨가 갑자기 추워졌는데, 건강 유의하시길 바라겠습니다.🙂

Signed-off-by: yongjunhong <kevin0928@naver.com>

seongahjo · 2024-12-28T04:39:42Z

fixture-monkey/src/test/java/com/navercorp/fixturemonkey/test/FixtureMonkeyOptionsTest.java

 	@Property
-	void registerSetFirst() {
-		String expected = "test2";
+	void registerWithPriority() {
 		FixtureMonkey sut = FixtureMonkey.builder()
-			.register(String.class, monkey -> monkey.giveMeBuilder("test"))
+			.register(String.class, monkey -> monkey.giveMeBuilder("test"), 1)
+			.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2)
 			.build();

 		String actual = sut.giveMeBuilder(String.class)
-			.set("$", expected)
 			.sample();

-		then(actual).isEqualTo(expected);
+		then(actual).isEqualTo("test");


아래 케이스에서도 잘 동작하는지 확인하는 테스트가 있으면 좋을 것 같습니다!

FixtureMonkey sut = FixtureMonkey.builder() .register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2) .register(String.class, monkey -> monkey.giveMeBuilder("test"), 1) .build();

아직 우선순위, 무작위 적용 관련 부분이 구현되지 않아서 해당 코드를 구현한 뒤 테스트 하겠습니다 :)

이제 이 테스트를 추가할 수 있을 것 같네요

테스트 코드를 추가한 결과 정상 작동하는 것을 확인했습니다.

다만 해당 케이스가 기존의 테스트 케이스와 어떤 점이 다른지 궁금합니다!

자세히는 register 연산의 순서만 바뀐 것으로 확인되는데, 이 것이 어떠한 것을 검증하는 테스트케이스인지 궁금합니다!

기존의 테스트 케이스입니다:

FixtureMonkey sut = FixtureMonkey.builder() .register(String.class, monkey -> monkey.giveMeBuilder("test"), 1) .register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2) .build();

새로운 테스트 케이스입니다:

FixtureMonkey sut = FixtureMonkey.builder() .register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2) .register(String.class, monkey -> monkey.giveMeBuilder("test"), 1) .build();

다만 해당 케이스가 기존의 테스트 케이스와 어떤 점이 다른지 궁금합니다!

아래 테스트를 추가하면 register의 선언 순서에 상관없이 우선순위에 따라 동작함을 보장할 수 있습니다.

아래 테스트를 추가하면 register의 선언 순서에 상관없이 우선순위에 따라 동작함을 보장할 수 있습니다.

확인했습니다!
해당 커밋에서 작업했습니다 478bb7a !

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

seongahjo · 2025-02-06T02:13:37Z

...re-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/MonkeyManipulatorFactory.java

+		return getHighestPriorityOperator(priorityOperators);
+	}
+
+	private DefaultArbitraryBuilder<?> getHighestPriorityOperator(


메서드 이름과 반환 값이 잘 매핑이 안되는 것 같습니다.

이 메서드는 제거하고, FixtureMonkey#getHighestPriorityOperators와 동일한 형태의 메서드를 추가하고 DefaultArbitraryBuilder 변환은 메서드 바깥에서 처리하는 건 어떨까요??

이름이 유사한데 동작이 달라서 일관성이 저하되는 것 같습니다.

좋은 지적 감사합니다 :)

해당 커밋에서 작업 했습니다! 0a34301

로컬에서 빌드 시 문제가 없는 것을 확인 했습니다!

seongahjo · 2025-02-06T02:14:51Z

...ure-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/PriorityMatcherOperator.java

+import com.navercorp.fixturemonkey.api.matcher.Matcher;
+import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;
+
+public final class PriorityMatcherOperator<T> extends MatcherOperator<T> {


아래 마킹이 들어가면 좋을 것 같습니다.
@API(since = "1.1.10", status = Status.EXPERIMENTAL)

가능하다면 주의사항이나 설계 의도를 주석에 적어주시면 좋을 것 같습니다.

해당 커밋에서 작업 했습니다! 36c424a

PriorityMatcherOperator 외 register 연산을 등록하는 API에도 동일하게 주석을 추가했습니다.

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

YongGoose · 2025-02-06T10:30:41Z

@seongahjo

해당 PR이 거의 끝난 것 같아서 앞으로의 일정에 대해 여쭤보고 싶어 댓글을 남깁니다!
register 연산 선택 작업이 3개의 PR로 나뉘어 약 6개월간 진행되면서, 이를 메인 브랜치에 병합할 때 conflict가 발생하는 상황입니다.

아래의 링크는 제가 Fixture Monkey 프로젝트에 해당 작업을 시작할 때 생성한 로드맵입니다.
https://yong-jun.notion.site/FixtureMonkey-31-a37b03716b1b4915b36eeb9a01f78153?pvs=74

로드맵에 따르면 아직도 많은 작업이 남았지만, 한 번 main 브랜치에 병합을 한 뒤, 나머지 작업을 하는 것도 괜찮아 보입니다.

로드맵을 업데이트한지 오랜시간이 지나 이 기능이 왜 필요하지..? 싶은 기능이 있을 수 있습니다..
성아님께서 추가하고 싶으신 기능이 있다면 자유롭게 의견 부탁드립니다.

성아님은 어떠한 생각을 가지고 계신지 궁금합니다!

추가적으로 Fixture Monkey Helper에서 selectName API에서 registerName을 편하게 검색하는 기능을 추가하고 싶은데 해당 프로젝트는 오픈소스인지 아닌지 여쭤보고 싶습니다!

현재는 변수명이 검색되지만, selectName의 경우 registerName이 검색되는 것이 맞는 것 같습니다.

만약 내부 프로젝트라면 해당 기능을 구현해주시면 감사할 것 같습니다!! 🙇🏻‍♂️

seongahjo · 2025-02-08T07:13:49Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java

+		return this;
+	}
+
+	public FixtureMonkeyBuilder registerGroup(


일단 이 옵션은 제공하지 않는 게 좋을 것 같습니다. 나중에 필요하면 추가하는 방향이 좋아보입니다.

좋은 것 같습니다.

해당 커밋에서 작업했습니다 8bacdc2 !

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

seongahjo · 2025-02-08T07:21:27Z

fixture-monkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/MatcherOperator.java

@@ -24,7 +24,7 @@
 import com.navercorp.fixturemonkey.api.property.Property;

 @API(since = "0.4.0", status = Status.MAINTAINED)
-public final class MatcherOperator<T> implements Matcher {
+public class MatcherOperator<T> implements Matcher {


API의 status를 INTERNAL로 변경하고, 주석으로 내부 사용을 위한 클래스임을 명시해주는 게 좋을 것 같습니다.

변경 완료 했습니다! 6f5e9f3

seongahjo · 2025-02-08T07:22:20Z

로드맵에 따르면 아직도 많은 작업이 남았지만, 한 번 main 브랜치에 병합을 한 뒤, 나머지 작업을 하는 것도 괜찮아 보입니다.

넵 좋은 것 같습니다! main 브랜치에 병합하고 변경이 되지 않을 최소한의 API를 포함해서 배포해서 사용해보는 것도 좋을 것 같습니다.

추가적으로 Fixture Monkey Helper에서 selectName API에서 registerName을 편하게 검색하는 기능을 추가하고 싶은데 해당 프로젝트는 오픈소스인지 아닌지 여쭤보고 싶습니다!

현재는 변수명이 검색되지만, selectName의 경우 registerName이 검색되는 것이 맞는 것 같습니다.

현재는 오픈소스가 아니긴 합니다. 어떻게 관리할지 아직 결정되지 않아서, 추후에 다시 공유드리겠습니다.
당장은 작업할 리소스가 없긴 해서 이부분도 나중에 다시 공유드리겠습니다.

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

YongGoose · 2025-02-08T07:31:31Z

넵 좋은 것 같습니다! main 브랜치에 병합하고 변경이 되지 않을 최소한의 API를 포함해서 배포해서 사용해보는 것도 좋을 것 같습니다.

감사합니다 🙂

현재는 오픈소스가 아니긴 합니다. 어떻게 관리할지 아직 결정되지 않아서, 추후에 다시 공유드리겠습니다.
당장은 작업할 리소스가 없긴 해서 이부분도 나중에 다시 공유드리겠습니다.

넵! 정해지면 공유 부탁드리겠습니다 🙇🏻‍♂️

seongahjo · 2025-02-08T07:42:47Z

...re-monkey/src/main/java/com/navercorp/fixturemonkey/customizer/MonkeyManipulatorFactory.java

+		if (registeredArbitraryBuilder != null) {
+			return registeredArbitraryBuilder;
+		}
+


코드만 보면 필드 타입을 동일한 name으로 registerName으로 등록한 경우 우선순위 적용이 안될 것 같습니다. 아래 테스트가 실패할 것 같은데 확인 부탁드립니다. 필드 타입을 register했을 때 테스트도 보강이 필요해보입니다.

FixtureMonkey sut = FixtureMonkey.builder() .registeredName( "test", String.class, monkey -> monkey.giveMeBuilder("test2"), 2 ) .registeredName( "test", String.class, monkey -> monkey.giveMeBuilder("test"), 1 ) .build(); String actual = sut.giveMeOne(StringObject.class).getValue(); then(actual).isEqualTo("test");

만약 말씀하시는 필드 타입이 registeredName (테스트 케이스에서는 test)이면 동일한 이름을 입력했을 때 예외가 발생합니다.
https://github.com/YongGoose/fixture-monkey/blob/6f5e9f3dd7cdf9848f6522362583026193bf0805/fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java#L450

해당 테스트에서 처리하고 있습니다.

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

YongGoose force-pushed the feature/random-selection branch from 477d170 to 06111d8 Compare November 29, 2024 10:57

seongahjo reviewed Dec 1, 2024

View reviewed changes

Implement random selection

b2abc78

Signed-off-by: yongjunhong <kevin0928@naver.com>

YongGoose force-pushed the feature/random-selection branch from 06111d8 to b2abc78 Compare December 1, 2024 08:37

YongGoose requested a review from seongahjo December 5, 2024 05:27

seongahjo reviewed Dec 6, 2024

View reviewed changes

Add priority feature

6e64284

Signed-off-by: yongjunhong <kevin0928@naver.com>

YongGoose force-pushed the feature/random-selection branch from c143264 to 6e64284 Compare December 6, 2024 16:09

YongGoose requested a review from seongahjo December 7, 2024 03:28

seongahjo reviewed Dec 7, 2024

View reviewed changes

YongGoose force-pushed the feature/random-selection branch from 5faa668 to 0a87f4e Compare December 13, 2024 05:28

Reduce code duplication

6fa32d7

Signed-off-by: yongjunhong <kevin0928@naver.com>

YongGoose force-pushed the feature/random-selection branch from 0a87f4e to 6fa32d7 Compare December 13, 2024 05:30

Refactor PriorityMatcherOperator

486cf5f

Signed-off-by: yongjunhong <kevin0928@naver.com>

YongGoose requested a review from seongahjo December 26, 2024 11:48

seongahjo reviewed Dec 28, 2024

View reviewed changes

YongGoose requested a review from seongahjo January 2, 2025 11:35

Add sorting mechanism for initialized instances

d00d7a7

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

seongahjo reviewed Feb 6, 2025

View reviewed changes

Add javadoc

36c424a

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

YongGoose marked this pull request as ready for review February 6, 2025 10:12

github-actions bot requested a review from acktsap February 6, 2025 10:13

github-actions bot requested review from benelog, jwChung, mhyeon-lee, seongahjo and SooKim1110 February 6, 2025 10:13

github-actions bot assigned YongGoose Feb 6, 2025

Apply comment

43e59aa

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

YongGoose force-pushed the feature/random-selection branch from 0a34301 to 43e59aa Compare February 6, 2025 10:19

seongahjo reviewed Feb 8, 2025

View reviewed changes

Apply comment

478bb7a

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

seongahjo reviewed Feb 8, 2025

View reviewed changes

YongGoose added 2 commits February 8, 2025 16:26

Remove registerGroup with priority method

8bacdc2

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

Update api status

6f5e9f3

Signed-off-by: yongjunhong <dev.yongjunh@gmail.com>

YongGoose requested a review from seongahjo February 8, 2025 07:38

seongahjo reviewed Feb 8, 2025

View reviewed changes

YongGoose requested a review from seongahjo February 8, 2025 08:15

		@@ -204,13 +206,18 @@ private void initializeRegisteredArbitraryBuilders(
		private void initializeNamedArbitraryBuilderMap(

		private final Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>>
		priorityGroupedMatchers = new HashMap<>();

Implement Random Selection of Register Operations When Not Using selectName #1108

Are you sure you want to change the base?

Implement Random Selection of Register Operations When Not Using selectName #1108

Conversation

YongGoose commented Nov 29, 2024 • edited Loading

Summary

Open Question

How Has This Been Tested?

Is the Document updated?

YongGoose commented Nov 29, 2024 • edited Loading

seongahjo commented Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

YongGoose Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose commented Dec 1, 2024

seongahjo commented Dec 1, 2024

YongGoose commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

seongahjo Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

seongahjo Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

YongGoose Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

YongGoose Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose commented Feb 6, 2025

seongahjo Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo commented Feb 8, 2025 • edited Loading

YongGoose commented Feb 8, 2025

seongahjo Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

YongGoose Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

YongGoose commented Nov 29, 2024 •

edited

Loading

YongGoose commented Nov 29, 2024 •

edited

Loading

seongahjo commented Dec 1, 2024 •

edited

Loading

YongGoose Dec 1, 2024 •

edited

Loading

YongGoose commented Dec 5, 2024 •

edited

Loading

YongGoose Dec 6, 2024 •

edited

Loading

seongahjo Dec 6, 2024 •

edited

Loading

seongahjo Dec 7, 2024 •

edited

Loading

seongahjo Dec 14, 2024 •

edited

Loading

YongGoose Dec 21, 2024 •

edited

Loading

seongahjo Jan 18, 2025 •

edited

Loading

YongGoose Jan 21, 2025 •

edited

Loading

YongGoose Dec 28, 2024 •

edited

Loading

seongahjo Feb 6, 2025 •

edited

Loading

YongGoose Feb 6, 2025 •

edited

Loading

seongahjo Feb 8, 2025 •

edited

Loading

seongahjo commented Feb 8, 2025 •

edited

Loading

seongahjo Feb 8, 2025 •

edited

Loading

YongGoose Feb 8, 2025 •

edited

Loading