-
Notifications
You must be signed in to change notification settings - Fork 79
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 ArbitraryValue methods return Arbitrary #39
Conversation
@@ -127,10 +128,10 @@ public ArbitraryGenerator getGenerator( | |||
@SuppressWarnings({"rawtypes", "unchecked"}) | |||
public Arbitrary<T> result( | |||
Supplier<Arbitrary<T>> generateArbitrary, | |||
ArbitraryValidator<T> validator, | |||
ArbitraryValidator validator, |
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.
ArbitraryValidator 인터페이스가 제네릭이 아니여도 될 것 같은데, 다음에 살펴보고 제거 가능하면 제거할 예정입니다
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.
넵 제거는 0.4.0 에 제거되면 될거 같습니다.
} | ||
|
||
@Override | ||
public synchronized Stream<T> sampleStream() { | ||
try { | ||
return getArbitrary().sampleStream(); | ||
} catch (TooManyFilterMissesException ex) { |
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.
sampleStream() 동작에서 객체 생성을 하지 않아서 TooManyFilterMissesException 가 발생하지는 않을거 같습니다.
Stream 에서 terminate 메소드인 .collect()
가 호출될 때 객체 생성이 발생하기 때문에 .collect
를 호출하는 클라이언트에서 TooManyFilterMissesException 를 받게되서 여기의 catch 가 의미가 없어집니다.
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.
그렇네요. collect 시점을 추적하는 건 불가능할 것 같아 revert 했습니다.
} | ||
public Arbitrary<T> edgeCases(Consumer<Config<T>> configurator) { | ||
return new ArbitraryValue<>( | ||
() -> getArbitrary().edgeCases(configurator), |
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.
getArbitrary() 를 호출할 때 Validation Filter 가 추가 되는데요
여기서 new ArbitraryValue() 객체 안에서 또다시 getArbitrary() 메소드가 불리면 추가로 Validation Filter 가 추가될거 같습니다.
반복적으로 Validation Filter 가 중첩되면서 sample 시 불필요하게 Validation 을 실행하게 될거 같습니다.
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.
넵, sample 시점에서만 실행하는 게 맞겠네요
@seongahjo 그리고 junit-platform-engine 에서 Junit Test 의 Exception 이 던져진걸 받아서 처리하는 callback 을 등록할 수 있습니다. |
@mhyeon-lee |
@SuppressWarnings("unchecked") | ||
private <U> Arbitrary<U> convert(Arbitrary<T> arbitrary, Function<Arbitrary<T>, Arbitrary<U>> mapper) { | ||
return mapper.apply(arbitrary.filter(validateFilter(validOnly))); | ||
} |
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 의 타입이 변한 후에는 validateFilter 안에 들어오더라도 문제가 없어서, 변환 전에 validation 하도록 추가했습니다.
아마 ArbitraryValidator가 CompositeArbitraryValidator일텐데 이상하네요.
ArbitraryValidator를 수정할 때 한 번에 수정하겠습니다.
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.
@seongahjo 음.. 변환을한 후에 최종적으로 validation 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.
@mhyeon-lee
변환 후에도 validation filter를 탑니다.
넵, 이전 커밋에서 변환후에만 validation filter를 탔는데, 이상하게 Validator에서 유효한 객체라고 판단을 합니다.
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.
generator, exhaustive 등을 통해서 반환된 값으로 sampling 할때는 validation 을 타지 못할 거 같습니다.
convert 로 filter 를 태우지 못하고 반환하는 메소드들에는 validation 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.
넵, 지금은 Arbitrary을 반환하는 경우만 처리했고, 그런 경우는 다음 PR에서 처리할 예정입니다.
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.
mapper.apply(arbitrary).filter(validateFilter(validOnly))
로 변경하시고
tuple1, tuple2, collect, flatMap 등 타입을 변환하는 연산 검증 실패 테스트를 실행해보시면 확인하실 수 있습니다.
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.
@seongahjo
tuple1, tuple2, collect, flatMap 등은 Validation 이 의도한 대로 안되는게 맞을거 같군요
이것들은 filter 를 먹이고 mapper 를 실행하는게 맞을거 같습니다.
저것들이 아닌 자기 자신의 타입으로 변환하는 것들은 mapping 후에 filter 를 적용해야 될거 같은데
ArbitraryValue 에서 sample 시 validation filter 가 추가되므로, filter 를 적용하지 않고 ArbitraryValue 를 생성 반환하는게 좋을거 같습니다.
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.
다음과 같이 정리될 수 있을거 같습니다.
tuple1, tuple2, collect, flatMap 과 같이 컨테이너 타입이 되는건 각 요소에 validation 을 적용해야 되기 때문에 filter 를 적용한 후 mapping 하는게 좋을거 같고
T -> T 또는 T -> U 인데 단일 객체로 mapping 되는 map 은 filter 를 적용하지 않고 ArbitraryValue 로 넘긴 후 거기서 sample 시 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.
@mhyeon-lee
넵, 말씀하신 대로 자기 자신의 타입으로 변환되는 경우에는 filter를 적용하지 않습니다.
filter를 적용하는 곳이 sample, convert 두 곳인데,
convert는 다른 타입으로 변환되는 경우에만 실행합니다.
ex) map에는 convert가 적용되지 않습니다.
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.
@seongahjo 아 이미 이렇게 구분되서 작업 되어있군요;; 제가 제대로 못보고 의견 드렸네요
수고하셨습니다. 👍
@seongahjo |
@mhyeon-lee |
boolean validOnly | ||
ArbitraryValidator validator, | ||
boolean validOnly, | ||
Map<String, ConstraintViolation> violations |
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.
이거는 왜 빈 Map 을 주입받도록 했나요?
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.
ArbitraryValue 에 map, filter 등의 연산이 실행될 때마다 새로운 인스턴스가 생성될텐데요.
sample을 실행하는 시점의 인스턴스와 sample을 호출받는 인스턴스가 다릅니다.
동일한 실행 흐름의 인스턴스간 violations을 공유하기 위해 map을 주입받습니다.
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.
@seongahjo 아 그렇네요 이해했습니다.
@seongahjo Recorder 설계시 고민해봐야겠네요 |
} | ||
public Arbitrary<T> ignoreException(Class<? extends Throwable> exceptionType) { | ||
return new ArbitraryValue<>( | ||
() -> getArbitrary().ignoreException(exceptionType), |
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.
여기랑 아래 dontShrink, edgeCases 는 convert 가 적용안된거 같습니다.
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.
이 경우는 타입을 변환 하지 않아서 sample 시점에서 정상적으로 검증합니다.
실행 결과를 검증하는 테스트가 있습니다.
fixture-monkey/fixture-monkey/src/test/java/com/navercorp/fixturemonkey/test/FixtureMonkeyTest.java
Line 544 in 4426ba7
void giveMeBuilderBuildEdgeCasesInvalidThenSampleThrowsTooManyFilterMissesException() { |
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.
@seongahjo 아 sample 시점에 검사하는게 다시 들어갔군요
그러면 convert 가 들어간거는 convert 할때 validation filter 가 적용되고 새로 만들어진 ArbitraryValue 에서 sample 을 하면 다시 validation 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.
<T>
-> <U>
라면 T 에서 한번, U 에서 한 번 검증합니다.
U에서 검증할 때 문제가 있어 보입니다.
우선 Arbitrary를 반환값으로 가지는 경우만 처리했습니다.
Arbitrary를 반환할 때 ArbitraryValue를 반환해서
sample
,sampleStream
을 실행하는 시점을 추적합니다.값을 반환하는
sample
,sampleStream
를 실행하는 시점에서 내부 동작은 모두 Lazy하고 모든 연산이 실행됩니다.Validation 실패 원인을 로깅하기 위해 같은 ArbitraryValue 객체들은 violations를 공유합니다.
동시에 실행하는 경우는 거이 없겠지만, 있을 수도 있으므로 ConcurrentHashMap을 구현체로 사용합니다.
공유하더라도 Validation 실패하는 경우이므로 문제가 없습니다.