Refactor ArbitraryValue methods return Arbitrary #39

seongahjo · 2021-10-02T16:40:06Z

우선 Arbitrary를 반환값으로 가지는 경우만 처리했습니다.
Arbitrary를 반환할 때 ArbitraryValue를 반환해서 sample, sampleStream을 실행하는 시점을 추적합니다.
값을 반환하는 sample, sampleStream를 실행하는 시점에서 내부 동작은 모두 Lazy하고 모든 연산이 실행됩니다.

Validation 실패 원인을 로깅하기 위해 같은 ArbitraryValue 객체들은 violations를 공유합니다.
동시에 실행하는 경우는 거이 없겠지만, 있을 수도 있으므로 ConcurrentHashMap을 구현체로 사용합니다.
공유하더라도 Validation 실패하는 경우이므로 문제가 없습니다.

seongahjo · 2021-10-02T16:40:47Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryTree.java

@@ -127,10 +128,10 @@ public ArbitraryGenerator getGenerator(
 	@SuppressWarnings({"rawtypes", "unchecked"})
 	public Arbitrary<T> result(
 		Supplier<Arbitrary<T>> generateArbitrary,
-		ArbitraryValidator<T> validator,
+		ArbitraryValidator validator,


ArbitraryValidator 인터페이스가 제네릭이 아니여도 될 것 같은데, 다음에 살펴보고 제거 가능하면 제거할 예정입니다

넵 제거는 0.4.0 에 제거되면 될거 같습니다.

mhyeon-lee · 2021-10-02T16:51:01Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java

 	}

 	@Override
 	public synchronized Stream<T> sampleStream() {
 		try {
 			return getArbitrary().sampleStream();
+		} catch (TooManyFilterMissesException ex) {


sampleStream() 동작에서 객체 생성을 하지 않아서 TooManyFilterMissesException 가 발생하지는 않을거 같습니다.

Stream 에서 terminate 메소드인 .collect() 가 호출될 때 객체 생성이 발생하기 때문에 .collect 를 호출하는 클라이언트에서 TooManyFilterMissesException 를 받게되서 여기의 catch 가 의미가 없어집니다.

그렇네요. collect 시점을 추적하는 건 불가능할 것 같아 revert 했습니다.

mhyeon-lee · 2021-10-02T16:54:08Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java

-		}
+	public Arbitrary<T> edgeCases(Consumer<Config<T>> configurator) {
+		return new ArbitraryValue<>(
+			() -> getArbitrary().edgeCases(configurator),


getArbitrary() 를 호출할 때 Validation Filter 가 추가 되는데요
여기서 new ArbitraryValue() 객체 안에서 또다시 getArbitrary() 메소드가 불리면 추가로 Validation Filter 가 추가될거 같습니다.

반복적으로 Validation Filter 가 중첩되면서 sample 시 불필요하게 Validation 을 실행하게 될거 같습니다.

넵, sample 시점에서만 실행하는 게 맞겠네요

mhyeon-lee · 2021-10-02T17:05:13Z

@seongahjo
제 생각에는 0.4.0 으로 가면서 아래 링크에서 제안해주신것처럼 밖에서 Validation Filter 를 걸고 TooManyFilter... 의 Violation 은 수집하지 않는 방향으로 가면 어떨까 싶습니다. (발생하면 그냥 던져집니다.)
#33 (comment)

그리고 junit-platform-engine 에서 Junit Test 의 Exception 이 던져진걸 받아서 처리하는 callback 을 등록할 수 있습니다.
fixture-monkey-engine 모듈에서 이 callback 을 등록해서 테스트 밖으로 던져진 TooManyFilter... Exception 을 받아 violation 등을 로깅하는게 어떨까 싶습니다.

seongahjo · 2021-10-02T17:09:26Z

@mhyeon-lee
violation을 로깅을 하기 위해서는 ConstraintViolationException 이 발생할 때 저장을 해야할텐데요.
TooManyFilterMissesException 에는 violation에 대한 정보가 없어서 로깅이 불가능하지 않나요?

seongahjo · 2021-10-02T17:19:24Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java

+	@SuppressWarnings("unchecked")
+	private <U> Arbitrary<U> convert(Arbitrary<T> arbitrary, Function<Arbitrary<T>, Arbitrary<U>> mapper) {
+		return mapper.apply(arbitrary.filter(validateFilter(validOnly)));
+	}


Arbitrary 의 타입이 변한 후에는 validateFilter 안에 들어오더라도 문제가 없어서, 변환 전에 validation 하도록 추가했습니다.
아마 ArbitraryValidator가 CompositeArbitraryValidator일텐데 이상하네요.
ArbitraryValidator를 수정할 때 한 번에 수정하겠습니다.

@seongahjo 음.. 변환을한 후에 최종적으로 validation filter 를 태워야 될거 같습니다.

@mhyeon-lee
변환 후에도 validation filter를 탑니다.
넵, 이전 커밋에서 변환후에만 validation filter를 탔는데, 이상하게 Validator에서 유효한 객체라고 판단을 합니다.

generator, exhaustive 등을 통해서 반환된 값으로 sampling 할때는 validation 을 타지 못할 거 같습니다.
convert 로 filter 를 태우지 못하고 반환하는 메소드들에는 validation filter 들을 추가해주시면 좋을거 같습니다.

넵, 지금은 Arbitrary을 반환하는 경우만 처리했고, 그런 경우는 다음 PR에서 처리할 예정입니다.

mapper.apply(arbitrary).filter(validateFilter(validOnly))로 변경하시고
tuple1, tuple2, collect, flatMap 등 타입을 변환하는 연산 검증 실패 테스트를 실행해보시면 확인하실 수 있습니다.

@seongahjo
tuple1, tuple2, collect, flatMap 등은 Validation 이 의도한 대로 안되는게 맞을거 같군요
이것들은 filter 를 먹이고 mapper 를 실행하는게 맞을거 같습니다.

저것들이 아닌 자기 자신의 타입으로 변환하는 것들은 mapping 후에 filter 를 적용해야 될거 같은데
ArbitraryValue 에서 sample 시 validation filter 가 추가되므로, filter 를 적용하지 않고 ArbitraryValue 를 생성 반환하는게 좋을거 같습니다.

다음과 같이 정리될 수 있을거 같습니다.

tuple1, tuple2, collect, flatMap 과 같이 컨테이너 타입이 되는건 각 요소에 validation 을 적용해야 되기 때문에 filter 를 적용한 후 mapping 하는게 좋을거 같고

T -> T 또는 T -> U 인데 단일 객체로 mapping 되는 map 은 filter 를 적용하지 않고 ArbitraryValue 로 넘긴 후 거기서 sample 시 filter 를 타게 하는게 좋을거 같습니다.

@mhyeon-lee
넵, 말씀하신 대로 자기 자신의 타입으로 변환되는 경우에는 filter를 적용하지 않습니다.
filter를 적용하는 곳이 sample, convert 두 곳인데,
convert는 다른 타입으로 변환되는 경우에만 실행합니다.

ex) map에는 convert가 적용되지 않습니다.

@seongahjo 아 이미 이렇게 구분되서 작업 되어있군요;; 제가 제대로 못보고 의견 드렸네요
수고하셨습니다. 👍

mhyeon-lee · 2021-10-02T17:30:01Z

@seongahjo
맞네요 제가 잘못 생각했었네요
다시 정리하면, 아래와 같이 하면 어떨까 싶습니다.
TooManyFilterMissesException 이거랑은 관계 없이 ConstraintViolationException 에러를 기록할 수 있는 Recorder 를 주입할 수 있게 하고 Recorder 에 기록합니다.
테스트가 끝났을 때 에러가 던져졌다면, callback 에서 Recorder 에 기록된 violation 들을 로깅하도록 합니다.

seongahjo · 2021-10-02T17:33:27Z

@mhyeon-lee
동일한 ArbitraryValue 클래스라 하더라도 인스턴스가 다르다면 ConstraintViolationException를 개별 인스턴스마다 기록을 해야할텐데요.
Recorder에서 이런 동작이 가능한가요?

mhyeon-lee · 2021-10-02T17:33:52Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java

-		boolean validOnly
+		ArbitraryValidator validator,
+		boolean validOnly,
+		Map<String, ConstraintViolation> violations


이거는 왜 빈 Map 을 주입받도록 했나요?

ArbitraryValue 에 map, filter 등의 연산이 실행될 때마다 새로운 인스턴스가 생성될텐데요.
sample을 실행하는 시점의 인스턴스와 sample을 호출받는 인스턴스가 다릅니다.
동일한 실행 흐름의 인스턴스간 violations을 공유하기 위해 map을 주입받습니다.

@seongahjo 아 그렇네요 이해했습니다.

mhyeon-lee · 2021-10-02T17:39:12Z

@seongahjo Recorder 설계시 고민해봐야겠네요

mhyeon-lee · 2021-10-02T17:51:33Z

fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java

-		}
+	public Arbitrary<T> ignoreException(Class<? extends Throwable> exceptionType) {
+		return new ArbitraryValue<>(
+			() -> getArbitrary().ignoreException(exceptionType),


여기랑 아래 dontShrink, edgeCases 는 convert 가 적용안된거 같습니다.

이 경우는 타입을 변환 하지 않아서 sample 시점에서 정상적으로 검증합니다.

실행 결과를 검증하는 테스트가 있습니다.

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

Line 544 in 4426ba7

void giveMeBuilderBuildEdgeCasesInvalidThenSampleThrowsTooManyFilterMissesException() {

@seongahjo 아 sample 시점에 검사하는게 다시 들어갔군요
그러면 convert 가 들어간거는 convert 할때 validation filter 가 적용되고 새로 만들어진 ArbitraryValue 에서 sample 을 하면 다시 validation filter 가 중첩으로 적용되나요?

<T> -> <U>라면 T 에서 한번, U 에서 한 번 검증합니다.

U에서 검증할 때 문제가 있어 보입니다.

Refactor ArbitraryValue methods return Arbitrary

f10568e

seongahjo requested a review from mhyeon-lee October 2, 2021 16:40

seongahjo self-assigned this Oct 2, 2021

seongahjo commented Oct 2, 2021

View reviewed changes

Fix test

3d646c3

mhyeon-lee reviewed Oct 2, 2021

View reviewed changes

Revert sampleStream

a0dd2be

mhyeon-lee reviewed Oct 2, 2021

View reviewed changes

seongahjo added 2 commits October 3, 2021 01:55

Fix checkstyle

15b77f9

Refactor validateFilter only on sample

24480b3

Fix test

4426ba7

seongahjo commented Oct 2, 2021

View reviewed changes

mhyeon-lee reviewed Oct 2, 2021

View reviewed changes

mhyeon-lee mentioned this pull request Oct 2, 2021

Add Recorder in fixture-monkey-engine #42

Open

mhyeon-lee reviewed Oct 2, 2021

View reviewed changes

mhyeon-lee approved these changes Oct 2, 2021

View reviewed changes

seongahjo merged commit 9616b08 into main Oct 2, 2021

seongahjo deleted the sa/refactor-arbitrary-value-with-arbitrary branch October 2, 2021 18:16

mhyeon-lee added this to the 0.3.1 milestone Oct 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ArbitraryValue methods return Arbitrary #39

Refactor ArbitraryValue methods return Arbitrary #39

seongahjo commented Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021

mhyeon-lee commented Oct 2, 2021 •

edited

seongahjo commented Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

mhyeon-lee commented Oct 2, 2021

seongahjo commented Oct 2, 2021

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021

mhyeon-lee Oct 2, 2021

mhyeon-lee commented Oct 2, 2021

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

mhyeon-lee Oct 2, 2021

seongahjo Oct 2, 2021 •

edited

Refactor ArbitraryValue methods return Arbitrary #39

Refactor ArbitraryValue methods return Arbitrary #39

Conversation

seongahjo commented Oct 2, 2021

seongahjo Oct 2, 2021 • edited

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

Choose a reason for hiding this comment

mhyeon-lee commented Oct 2, 2021 • edited

seongahjo commented Oct 2, 2021

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhyeon-lee commented Oct 2, 2021

seongahjo commented Oct 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhyeon-lee commented Oct 2, 2021

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 • edited

Choose a reason for hiding this comment

seongahjo Oct 2, 2021 •

edited

mhyeon-lee commented Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited

seongahjo Oct 2, 2021 •

edited