naver
Merge 31-register branch into main and document changes
Summary
Merge three PRs into the main branch and document changes.
- https://github.com/naver/fixture-monkey/pull/1036
- https://github.com/naver/fixture-monkey/pull/1062
- https://github.com/naver/fixture-monkey/pull/1108
How Has This Been Tested?
- existing tests
Is the Document updated?
- I plan to do it in this PR.
@seongahjo
다른 업무를 하다 보니 PR 생성이 늦어졌네요.. 😅 궁금한 점이 있어 우선, Draft PR을 생성한 뒤 작업을 추가적으로 해보려고 합니다.
ObjectBuilder와ArbitraryBuilder의 차이점이 무엇인가요?
또한, 최근 작업 내용 상황을 잘 모르다보니 성아님께서 삭제하기로 결정하신 클래스를 사용한 부분이 있을 수도 있습니다. 혹시 시간 괜찮으실 때 전체적으로 피드백 한 번 부탁드려도 될까요?
ObjectBuilder와 ArbitraryBuilder의 차이점이 무엇인가요?
fixture-monkey-api에서 fixture-monkey의 ArbitraryBuilder를 참조하기 위해 사용하는 마킹 인터페이스입니다.
@seongahjo
해당 커밋에서 https://github.com/naver/fixture-monkey/pull/1166/commits/4c97b304a7b7244dfec58f6fe07ef3d200d43cd5 FixtureMonkey의 필드로 존재하던 registeredArbitraryBuilders을 제거하고 monkeyContext의 필드를 사용하도록 리팩토링 했습니다.
MonkeyContext의registeredArbitraryBuilders는PriorityMatcherOperator으로 변경 했습니다!
해당 부분 한 번 검토 부탁드립니다!
만약 문제가 없다면 문서 작업으로 넘어가려 합니다 :)
같이 얘기해보면 좋은 주제가 있어 공유합니다!
현재 FixureMonkey의 jdk baseline은 8로 알고 있는데요, 최근 많은 오픈소스 라이브러리들이 jdk baseline을 17로 올리려고 하는 추세로 보입니다. 그래서 Fixture Monkey도 이에 맞춰 baseline을 17로 올리면 어떨까 하는 생각이 듭니다.
@seongahjo 어떻게 생각하시나요?
만약 이미 Fixture monkey에서 17을 지원하거나 해당 계획이 없으시다면 무시하셔도 됩니다..ㅎㅎ
- https://github.com/junit-team/junit5/issues/4246
- https://github.com/cucumber/cucumber-jvm/issues/2962
- https://spring.io/blog/2022/03/28/an-update-on-java-17-adoption
@YongGoose
현재 FixureMonkey의 jdk baseline은 8로 알고 있는데요, 최근 많은 오픈소스 라이브러리들이 jdk baseline을 17로 올리려고 하는 추세로 보입니다. 그래서 Fixture Monkey도 이에 맞춰 baseline을 17로 올리면 어떨까 하는 생각이 듭니다.
JDK 버전을 올리려는 계획이 있었긴 했는데요, JDK8 유저를 포기할만큼 큰 장점은 찾지는 못해서 보류해왔었습니다. 다른 라이브러리들이 올리려는 계획이 있다면, 다시 한 번 검토해볼 때가 된 것 같네요.
좋은 의견 감사합니다!
@seongahjo
selectName 관련해서 한 가지 논의를 해봐야 하는 내용이 있어 코멘트 남깁니다.
@Property
void registerNestedSelectLatter() {
// given
Integer expectedInt = 1;
FixtureMonkey sut = FixtureMonkey.builder()
.registerByName(
"simpleObject",
SimpleObject.class,
monkey -> monkey.giveMeBuilder(SimpleObject.class)
.set("wrapperInteger", expectedInt)
)
.registerByName(
"string",
String.class,
monkey -> monkey.giveMeBuilder("simpleObject")
)
.build();
// when
Integer actual = sut.giveMeBuilder(SimpleObject.class)
.selectName("simpleObject")
.selectName("string")
.sample()
.getWrapperInteger();
then(actual).isEqualTo(expectedInt);
}
현재 위 테스트 케이스는 SimpleObject의 wrapperInteger 필드를 1로 변경하는 A 연산과, string 필드를 "simpleObject"로 변경하는 B 연산을 적용한 경우를 다루고 있습니다.
이때 A 연산에서 변경한 값이 정상적으로 적용이 됐는지 확인해보려고 합니다.
하지만 제 의도와 다르게 A 연산을 적용한 뒤, B 연산을 적용하면 A 연산에서 적용한 값들이 무시가 되는 것 같습니다.
아래의 이미지는 A 연산과 B 연산을 각각 selectName API를 활용해 선택을 한 뒤, 생성이 된 Manipulator의 값입니다.\
- A 연산의 값이 무시된 것을 확인할 수 있습니다.
하지만 이 테스트 케이스를 아래와 같이 변경을 하면 정상적으로 테스트가 됩니다.
.selectName("simpleObject", "string")
작성해주신 코멘트도 이와 연관이 있다고 생각합니다.
- https://github.com/naver/fixture-monkey/pull/1166#discussion_r2038778611
.selectName(A,B)와 .selectName(A).selectName(B)는 동일하다고 봤을 때, 직관적으로는 selectName(B)가 적용이 되야할 것 같습니다.
현재의 경우 .selectName(A,B)와 .selectName(A).selectName(B) 은 동일하지 않습니다.
제 생각에는 .selectName(A,B)와 .selectName(A).selectName(B) 경우가 동일해야 한다고 생각하는데요, selectName에서 매번 ArbitraryBuilderContext를 초기화 하는 로직을 수정해야 할 것 같습니다.
ArbitraryBuilderContext builderContext = monkeyContext.getRegisteredArbitraryBuilders()
.stream()
.filter(operator -> Arrays.stream(names)
.anyMatch(name -> operator.match(rootProperty, NamedMatcher.metadata(name)))
)
.map(MatcherOperator::getOperator)
.findAny()
.map(DefaultArbitraryBuilder.class::cast)
.map(DefaultArbitraryBuilder::getContext)
.orElse(ArbitraryBuilderContext.newBuilderContext(monkeyContext));
DefaultArbitraryBuilder에서ArbitraryBuilderContext를 필드로 가지고 있는 것도 하나의 방법이 될 수 있습니다.
작성하다 보니 코멘트가 길어졌습니다. 내용이 잘 전달되지 않았다면, 구글 미트나 커피 챗 등으로 논의하는 것도 좋을 것 같습니다. ㅎㅎ ☕️
@YongGoose 연휴 이후에 시간 괜찮으실 때 커피챗 한 번 하는 것도 좋을 것 같네요.
괜찮으시다면 아래 메일로 연락주시면 좋을 것 같습니다!
@seongahjo
현재 아래의 테스트가 실패하고 있습니다. (upstream의 최신 main 브랜치에서도 실패하는 상황입니다)
저는 개인적으로 InnerSpec의 버그라고 생각하는데요, 어떻게 생각하시나요?
해당 커멘트도 버그로 인해 실패하는 것이라고 의심이 됩니다.
- https://github.com/naver/fixture-monkey/pull/1166#discussion_r2093217287
minSize comment에 따르면 minSize로 주어진 값이 컨테이너의 최소 크기인데, 현재 RegisterGroup에는 해당 값이 10으로 설정되어 있습니다. 하지만 생성되는 컨테이너의 size를 확인해보면 10을 넘는 경우가 없습니다.
만약 성아님도 버그로 판단하시면, 이슈를 발행해 커뮤니티 멤버들이 기여할 수 있도록 조치하겠습니다!
실패하는 테스트
@Property
void registerGroup() {
FixtureMonkey sut = FixtureMonkey.builder()
.registerGroup(RegisterGroup.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(1, 3);
then(actual2).hasSizeGreaterThan(9);
then(actual3.getIntValue()).isEqualTo(RegisterGroup.FIXED_INT_VALUE.getIntValue());
}
RegisterGroup.java
public static class RegisterGroup {
public static final ConcreteIntValue FIXED_INT_VALUE = new ConcreteIntValue();
public ArbitraryBuilder<String> string(FixtureMonkey fixtureMonkey) {
return fixtureMonkey.giveMeBuilder(String.class)
.set(Arbitraries.strings().numeric().ofMinLength(1).ofMaxLength(3));
}
public ArbitraryBuilder<List<String>> stringList(FixtureMonkey fixtureMonkey) {
return fixtureMonkey.giveMeBuilder(new TypeReference<List<String>>() {
})
.setInner(
new InnerSpec()
.minSize(10)
);
}
public ArbitraryBuilder<ConcreteIntValue> concreteIntValue(FixtureMonkey fixtureMonkey) {
return fixtureMonkey.giveMeBuilder(FIXED_INT_VALUE);
}
}
@YongGoose 님 테스트 실패가 naver/fixture-monkey main이 아니라 fork하신 레포의 main에서 실패하고 있는 것으로 보입니다.
아래 5 커밋 중에 영향을 준 커밋이 있을지 확인이 필요해보입니다.
@YongGoose 님 테스트 실패가
naver/fixture-monkeymain이 아니라 fork하신 레포의 main에서 실패하고 있는 것으로 보입니다.아래 5 커밋 중에 영향을 준 커밋이 있을지 확인이 필요해보입니다.
현재 실패하는 테스트 중 하나는 해결했습니다! 말씀해주신대로 기존의 5개의 커밋 중, 영향을 준 커밋이 있는지 한 번 확인해보겠습니다!
다만 naver/fixture-monkey의 main 브랜치 최신 상태를 기준으로 새 브랜치를 생성해 테스트를 수정했음에도, 동일한 문제가 발생해 공유드립니다!
https://github.com/YongGoose/fixture-monkey/commit/74dfc91b4dcdba22bc267ed81293c004b970d43b
위 커밋에서는 then(actual2).hasSizeLessThan(5); 의 값을 3으로 변경했습니다.
RegisterGroup 클래스의 stringList method에서 maxSize를 2로 설정했기 때문에 실패하면 안 되는 케이스입니다.
public ArbitraryBuilder<List<String>> stringList(FixtureMonkey fixtureMonkey) {
return fixtureMonkey.giveMeBuilder(new TypeReference<List<String>>() {
})
.setInner(
new InnerSpec()
.maxSize(2)
);
}
size method에 전달되는 minSize, maxSize 파라미터 값을 확인해도 0, 2 정상적으로 받는 것을 확인했습니다.
그러나 list의 크기가 3으로 설정이 됩니다.
@YongGoose 님 register 에 있던 버그가 맞네요. active와 standby로 분리하면서 생긴 예외 케이스로 보입니다.
아래 PR에서 수정됐습니다. 확인 감사합니다 👍
https://github.com/naver/fixture-monkey/pull/1189
@YongGoose 님
register에 있던 버그가 맞네요. active와 standby로 분리하면서 생긴 예외 케이스로 보입니다. 아래 PR에서 수정됐습니다. 확인 감사합니다 👍#1189
감사합니다 :) 확인을 해본 결과 우선순위가 현재 적용이 되지 않는 것 같은데 밤에 다시 확인해본 뒤, 코멘트 남기겠습니다!
- 우선순위에 상관 없이 무조건 처음에 위치한 연산이 선택됩니다.
감사합니다 :) 확인을 해본 결과 우선순위가 현재 적용이 되지 않는 것 같은데 밤에 다시 확인해본 뒤, 코멘트 남기겠습니다!
- 우선순위에 상관 없이 무조건 처음에 위치한 연산이 선택됩니다.
@seongahjo https://github.com/naver/fixture-monkey/pull/1166/commits/27113528871f9cfbef135d8c7b8d9135f55e338b 해당 커밋에서 작업했습니다!
그리고 모든 작업이 끝나 전체적으로 확인하신 뒤, 피드백 주시면 감사할 것 같습니다!! 긴 여행이었네요... 덕분에 많이 성장하고 배웠습니다 🙇🏻♂️
다음 작업에서도 최선을 다해 Fixture Monkey에 도움이 될 수 있도록 노력하겠습니다!
아래 세 작업만 마무리되면 될 것 같습니다. 긴 시간동안 수고하셨습니다 👍
- ExperimentalArbitraryBuilder로 selectName를 옮기기
- registeredRootTreeManipulator 처리 확인 (findFirst를 추가한 이유 확인)
- 코틀린에서 정상 동작하는지 확인하는 테스트 추가, selectName의 적용 순서를 확인하는 테스트 추가
ex, selectName의 적용 순서를 확인하는 테스트
@Property
void registerNestedSelectLatter() {
// given
String expected = "string";
FixtureMonkey sut = FixtureMonkey.builder()
.registeredName(
"simpleObject",
SimpleObject.class,
monkey -> monkey.giveMeBuilder(SimpleObject.class)
.set("str", "simpleObject")
)
.registeredName(
"string",
String.class,
monkey -> monkey.giveMeBuilder(expected)
)
.build();
// when
String actual = sut.giveMeBuilder(SimpleObject.class)
.selectName("simpleObject", "string")
.sample()
.getStr();
then(actual).isEqualTo(expected);
}
@Property
void registerNestedSelectFormer() {
// given
String expected = "simpleObject";
FixtureMonkey sut = FixtureMonkey.builder()
.registeredName(
"simpleObject",
SimpleObject.class,
monkey -> monkey.giveMeBuilder(SimpleObject.class)
.set("str", expected)
)
.registeredName(
"string",
String.class,
monkey -> monkey.giveMeBuilder("string")
)
.build();
// when
String actual = sut.giveMeBuilder(SimpleObject.class)
.selectName("string", "simpleObject")
.sample()
.getStr();
then(actual).isEqualTo(expected);
}
@seongahjo
set의 경우 activeContext에 바로 추가하는 구조여서, 수정이 필요할 것 같습니다.
- set 내부에서
ArbitraryManipulator가 아닌PriorityMatcherOperator<ArbitraryBuilderContext>을 생성한 뒤,standbyContext에 추가 - set의 경우 우선순위 및 name이 적용되지 않는다고 명시
https://github.com/naver/fixture-monkey/pull/1166/commits/3a42c843e369d829801acaaa2c68ea286b89d654 해당 커밋과 같이 set이 아닌 giveMeBuilder를 통해 value를 전달한 경우, 두 개의 테스트 케이스 모두 정상 작동하는 것을 확인했습니다.
정리하자면, selectName의 적용 순서를 확인하는 테스트는 성공하였지만, set의 경우 논의가 필요하다고 생각합니다!
@YongGoose 님 이 기능이 정식 기능으로 나가기 위해서는 위 두 가지 테스트의 요구사항을 만족해야할 것 같습니다.
제안주신 1번 방법은 set이 정해진 범위를 넘어서 동작하도록 변경하는 것으로, 기존 코드와의 일관성을 해치게 됩니다. 이런 변경은 지금은 문제가 안되더라도 코드 확장에 문제가 될 가능성이 크다고 생각합니다.
제가 생각하기에 가장 적절한 방법은 재귀적인 관점에서 해결해야할 것 같습니다. 가장 최하위 타입의 standbyContext를 적용할 때(지금의 경우는 String) selectName 연산의 순서를 고려해야 할 것 같습니다.
NodeSetDecomposedValueManipulator의 forced 를 레퍼런스로 한 번 고민해보시면 어떨까 싶습니다.
필요하시면 머지하시고 새로운 PR에서 작업하셔도 좋을 것 같습니다.
머지 필요하시면 말씀해주세요.
제안 감사합니다 ㅎㅎㅎ 하지만 아직은 머지할 때가 아닌 것 같습니다! 말씀해주신 두 가지 테스트의 요구사항을 완벽히 만족할 때 머지 요청드리겠습니다!
저녁에 한번 자세히 분석을 해볼 계획입니다! 도움이 필요하면 연락 드리겠습니다
감사합니다 :)
@YongGoose 님 이 기능이 정식 기능으로 나가기 위해서는 위 두 가지 테스트의 요구사항을 만족해야할 것 같습니다.
제안주신 1번 방법은 set이 정해진 범위를 넘어서 동작하도록 변경하는 것으로, 기존 코드와의 일관성을 해치게 됩니다. 이런 변경은 지금은 문제가 안되더라도 코드 확장에 문제가 될 가능성이 크다고 생각합니다.
제가 생각하기에 가장 적절한 방법은 재귀적인 관점에서 해결해야할 것 같습니다. 가장 최하위 타입의 standbyContext를 적용할 때(지금의 경우는 String) selectName 연산의 순서를 고려해야 할 것 같습니다.
NodeSetDecomposedValueManipulator의 forced 를 레퍼런스로 한 번 고민해보시면 어떨까 싶습니다.
혹시 조금 더 자세한 설명 부탁드려도 될까요?
- 설계 쉽지 않네요... ㅎㅎ 😂
개인적으로 두 가지 접근법을 생각해보았습니다.
-
ArbitraryResolver의resolve메소드 내에서objectTree.generate()를 호출하기 전에, 트리를 순회하며 각ObjectNode에 대해 적용할 조작을 결정하고 적용하는 새로운 단계를 추가하려고 합니다. -
forced와 같이 정의된 우선순위에 따라 가장 높은 우선순위의 조작 하나만 적용합니다. (forced와 같이 특정 조건에서 값을 강제로 적용)- 이 방법의 경우 "우선순위라는 정보를 어디까지 알고 있어야 하나" 라는 것도 하나의 고민되는 부분이었습니다.
@YongGoose 깊게 고민하고 공유드린 건 아니고, 각 타입마다 재귀적으로 생성하므로 타입이 가지는 개별 context에서 결정이 가능하면 좋을 것 같다는 의견을 드린거긴 합니다. 다만 개별 케이스의 조건을 넣어서 예외처리하는 경우는 피하는 게 좋을 것 같습니다. 더 간단히 직관적으로, "if 없이 일반화할 수 있는 로직 추가로 해결"라고 이해해주셔도 좋을 것 같습니다. (부득이한 경우 처리를 위해 if가 들어갈 수 있을 것 같긴 합니다.)
monkeyManipulatorFactory.newRegisteredArbitraryManipulators(
monkeyContext.getRegisteredArbitraryBuilders(),
objectTree.getMetadata().getNodesByProperty()
);
ArbitraryResolver의 resolve 메소드 내에서 objectTree.generate()를 호출하기 전에, 트리를 순회하며 각 ObjectNode에 대해 적용할 조작을 결정하고 적용하는 새로운 단계를 추가하려고 합니다.
이 작업을 현재 register 연산을 만들기 위해서 하고 있습니다. 아래 코드가 말씀하신 작업을 하고 있는 코드입니다.
monkeyManipulatorFactory.newRegisteredArbitraryManipulators(
monkeyContext.getRegisteredArbitraryBuilders(),
objectTree.getMetadata().getNodesByProperty()
);
이 구현에 추가할 수 있으면 좋을 것 같긴 하네요.
forced와 같이 정의된 우선순위에 따라 가장 높은 우선순위의 조작 하나만 적용합니다. (forced와 같이 특정 조건에서 값을 강제로 적용)
우선순위는 단순히 숫자로만 표현하는 게 좋을 것 같습니다. 즉, 우선순위 정보 -> 우선순위 값을 만들어내는 별도의 책임을 담당하는 객체가 있다고 가정하고 진행하시는 게 가장 편하실 것 같습니다.
우선순위 값으로 위 조건을 만족할 수 있는 구현이 가능할지부터 확인해보시면 좋을 것 같습니다.
@YongGoose 님 다른 작업으로 인해 계속 rebase하셔야할 것 같은데, 일단 머지하고 다른 PR에서 작업하는 건 어떨까요??
사용자에게 노출되는 selectName api만 일단 제거해두려고 합니다.
@YongGoose 님 다른 작업으로 인해 계속 rebase하셔야할 것 같은데, 일단 머지하고 다른 PR에서 작업하는 건 어떨까요??
사용자에게 노출되는
selectNameapi만 일단 제거해두려고 합니다.
넵! Issue, PR 새로 생성한 뒤 작업 이어가겠습니다!