Add register and select ArbitraryBuilder by name

[YongGoose]

Summary

• Implement registration of ArbitraryBuilders with names.
• Implement selection of ArbitraryBuilder by name.

(Optional): Description

• Added a method registeredName to FixtureMonkeyBuilder to allow registering an ArbitraryBuilder with an arbitrary name.
• Added a selectName method to ArbitraryBuilder to enable selection by name.

How Has This Been Tested?

• new tests

Is the Document updated?

later

[seongahjo]

31-register branch를 target 브랜치로 설정해주시면 좋을 것 같습니다!

[seongahjo]

테스트를 추가해서 의도대로 동작하는지 확인해주시면 좋을 것 같습니다.

[YongGoose]

테스트를 추가해서 의도대로 동작하는지 확인해주시면 좋을 것 같습니다.

FixtureMonkeyBuilder의 API를 이용해 이름과 함께 ArbitraryBuilder를 등록했을 때 개수와, map의 key를 비교하는 방식으로 구현했습니다.
또한 이름이 겹치면 예외가 발생하기 때문에 해당 부분도 테스트 구현했습니다.

[seongahjo]

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

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

[seongahjo]

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

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

[YongGoose]

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

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

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

[seongahjo]

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

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

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

[YongGoose]

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

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

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

[seongahjo]

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

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

[YongGoose]

@seongahjo
성아님 Select 기능을 구현하는데 있어 궁금한 점이 생겨 코멘트 남깁니다.

하나의 FixtureMonkey을 통해 여러 번 테스트를 진행할 때 이전에 선택한 register한 연산이 남아있어도 되는지 궁금합니다!
하기와 같은 방식으로 구현을 하면 registeredArbitraryBuilders에 register한 연산을 추가합니다.
그로 인해 FixtureMonkey를 재 사용 해서 테스트를 하게 되면 이전에 등록한 register한 연산이 남아있습니다.

public final class DefaultArbitraryBuilder<T> implements ArbitraryBuilder<T>, ExperimentalArbitraryBuilder<T> {

	...
	
	@Override
	public ArbitraryBuilder<T> selectName(String... names) {
		for (String name : names) {
			MatcherOperator<? extends ArbitraryBuilder<?>> namedArbitraryBuilder = namedArbitraryBuilderMap.get(name);

			if (namedArbitraryBuilder == null) {
				throw new IllegalArgumentException("Given name is not registered. name: " + name);
			}
			registeredArbitraryBuilders.add(namedArbitraryBuilder);
		}
		return this;
	}

registeredArbitraryBuilders의 size가 2인 것을 볼 수 있습니다.

---

테스트는 격리가 중요해 하나의 테스트가 다른 테스트에 영향을 끼치지 않도록 보장하듯이 FixtureMonkey도 이전에 선택한 register한 연산이 다른 FixtureMonkey에 영향을 끼치면 안 된다고 생각합니다.

그래서 ArbitraryBuilder에서 register한 연산을 선택할 때 이전에 저장된 register한 연산을 모두 지운 뒤, 저장하는 로직으로 구현을 하는 것이 방법이 될 수 있는데 성아님의 의견은 어떠신지 궁금합니다!

하기의 코드로 동일한 테스트에서 디버깅을 했을 때 registeredArbitraryBuilders의 size는 1이 나왔습니다.

	@Override
	public ArbitraryBuilder<T> selectName(String... names) {
               // clear을 통해 registeredArbitraryBuilders를 초기화 시킨 뒤 진행합니다.
		registeredArbitraryBuilders.clear();
		for (String name : names) {
			MatcherOperator<? extends ArbitraryBuilder<?>> namedArbitraryBuilder = namedArbitraryBuilderMap.get(name);

			if (namedArbitraryBuilder == null) {
				throw new IllegalArgumentException("Given name is not registered. name: " + name);
			}
      ...

[seongahjo]

@YongGoose

하나의 FixtureMonkey을 통해 여러 번 테스트를 진행할 때 이전에 선택한 register한 연산이 남아있어도 되는지 궁금합니다!

이전에 선택한 register 연산이 남아있으면 안될 것 같습니다. 지금은 큰 문제는 없을 것 같은데, 후에 기능을 확장할 때 예상치 못한 side-effect가 발생할 수 있어서요. 예를 들어서, 하나의 타입에 여러 register 연산을 허용하는 기능으로 확장할 계획도 가지고 있는데, 이때 문제가 될 수 있을 것 같습니다.

그래서 ArbitraryBuilder에서 register한 연산을 선택할 때 이전에 저장된 register한 연산을 모두 지운 뒤, 저장하는 로직으로 구현을 하는 것이 방법이 될 수 있는데 성아님의 의견은 어떠신지 궁금합니다!

이렇게 구현하면 registeredName만 있을 때는 문제가 없을텐데, register가 하나라도 있으면 다음 객체를 생성할 때 문제가 생길 것 같습니다. register와 registeredName이 같이 있을 때도 이슈가 없다는 게 보장된 방향으로 진행이 되야할 것 같습니다.

제 생각에는 ArbitraryBuilder 스코프에서 결정된 register 연산 (selectName)은 해당 스코프 내에서만 사용하고 제거될 수 있게 구현하는 방향이 좋을 것 같습니다.

[YongGoose]

@seongahjo

제 생각에는 ArbitraryBuilder 스코프에서 결정된 register 연산 (selectName)은 해당 스코프 내에서만 사용하고 제거될 수 있게 구현하는 방향이 좋을 것 같습니다.

좋은 방법인 것 같습니다!

selectName을 통해 선택한 register 연산을 해당 스코프 내에서만 사용하고 제거하기 위해 ArbitraryResolver의 resolve 메서드에서 registeredArbitraryBuilders를 깊은 복사 하여 registeredArbitraryBuildersCopy를 생성했습니다. 그런 뒤, 선택된 register 연산을 registeredArbitraryBuildersCopy에 추가하는 방식으로 구현했습니다.

테스트 결과 하기의 상황에서 정상적으로 동작함을 확인했습니다.

• register과 registeredName이 존재하는 경우 이슈가 없음
• 이전에 선택된 register 연산이 해당 스코프 내에서만 적용 됨
• sampleList의 결과값에 register 연산이 적용 됨

[seongahjo]

�컨벤션이 일관되지 않은 것 같은데 한 번 점검 부탁드립니다!

[YongGoose]

�컨벤션이 일관되지 않은 것 같은데 한 번 점검 부탁드립니다!

넵! 컨벤션 관련 부분 다시 확인하도록 하겠습니다.

[seongahjo]

ArrayList를 직접 사용하지 않고 List 인터페이스를 사용하면 좋을 것 같습니다.

지금 PR이 너무 길어지고 있는데, 기본적인 기능은 구현이 된 것 같으므로 머지하고 다음 PR에서 이 부분을 copy하지 않고 구현하는 방향으로 변경되면 좋을 것 같습니다.

[seongahjo]

제가 생각하는 방향은 registeredArbitraryBuilders와 namedArbitraryBuilder를 두 개로 관리하지 않고 하나의 변수로 관리하는 방향입니다.

구체적으로 말씀드리면, MatcherOperator 를 확장해서 위의 두 관심사 (property로 식별, name+property로 식별)을 모두 처리하는 방향입니다. 용준님도 이 방향이 괜찮을지 의견 주시면 좋을 것 같습니다.

[YongGoose]

ArrayList를 직접 사용하지 않고 List 인터페이스를 사용하면 좋을 것 같습니다.

넵! 수정 하겠습니다.

[YongGoose]

registeredArbitraryBuilders와 namedArbitraryBuilder를 두 개로 관리하지 않고 하나의 변수로 관리하는 방향입니다.

하나의 변수로 관리하는 방향은 좋은 것 같습니다.

MatcherOperator 를 확장해서 위의 두 관심사 (property로 식별, name+property로 식별)을 모두 처리하는 방향입니다.

괜찮은 것 같습니다.
조금 더 생각을 해본 뒤, Issue를 생성하고 다음 PR 진행하겠습니다. (더 괜찮은 방법이 생각나면 연락 드리겠습니다)

[seongahjo]

긴 시간동안 수고하셨습니다!!

기능은 거이다 구현이 된 것 같고, 컨벤션 관련해서 코멘트 드린 것만 반영해주시면 바로 머지하겠습니다.