-
Notifications
You must be signed in to change notification settings - Fork 80
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
Fix ArbitraryBuilder generated Arbitrary not affected Validator any with action #33
Fix ArbitraryBuilder generated Arbitrary not affected Validator any with action #33
Conversation
mhyeon-lee
commented
Oct 2, 2021
- ArbitraryBuilder 에서 바로 sample 을 하거나 build 로 반환된 Arbitrary 에서 sample 을 호출할 때는 문제가 없습니다. (일반적인 사용방법)
- build 된 Arbitrary 에서 sampleStream 이나 다른 액션을 통해 반환 받은 객체로 sampling 을 하면 Validation 을 안타게 됩니다.
- 그래서 ArbitararyGenerator 에서 내부 Arbitrary 를 생성할 때 filter 로 validation 을 추가해 항상 Validation 을 타도록 합니다.
- ArbitraryBuilder 가 build 시 반환하는 Arbitrary 를 관리하는 방식을 변경하긴 해야될거 같습니다.
- 다행히 공개 클래스는 아니라서 구조 변경에 자유롭긴 합니다.
@Example | ||
void giveMeBuilderInvalidThenSampleStreamTooManyFilterMissesException() { | ||
Arbitrary<StringWithNotBlankWrapperClass> sut = this.sut.giveMeBuilder(StringWithNotBlankWrapperClass.class) | ||
.set("value", "") |
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.
@NotBlank
가 마킹된 value 에 "" 를 넣어 Validation 과 상충되게 조건을 넣습니다.
조건에 따라 생성된 값이 Validation 을 통과할 수 없으므로, TooManyFilterMissesException 가 발생해야 합니다.
@mhyeon-lee |
@seongahjo 기존에는 아래 sampleStream 을 호출하는 테스트코드가 Validation 을 타지 않아서 blank 값이 들어간 객체가 생성됩니다. Validation Filter 를 Arbitrary 를 생성하는 시점에 셋팅해서 항상 Validation Filter 를 타도록 수정합니다. |
ccffec9
to
ec5c630
Compare
} | ||
|
||
@Override | ||
public Arbitrary<T> edgeCases(Consumer<Config<T>> configurator) { | ||
return getArbitrary().edgeCases(configurator); | ||
public synchronized Arbitrary<T> edgeCases(Consumer<Config<T>> 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.
드디어 synchronized가 필요한 경우가 발생했나요?
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.
각 메소드 호출후 Delegate 용 Arbitrary 를 다시 생성하기위해 null 로 셋팅 합니다
상태값을 변경하므로 동시성 호출에 문제가 없도록 synchronize 를 걸었습니다
보통 사용자가 하나의 ArbitraryValue 에 병렬 호출을 하지 않을거 같지만
사용자의 선택범위이니 동시성 방어를 해주는게 좋을거 같습니다
@mhyeon-lee 아 밖으로 뺀게 아니군요 |
@Override | ||
public Stream<T> sampleStream() { | ||
return getArbitrary().sampleStream(); | ||
public synchronized Stream<T> sampleStream() { | ||
try { | ||
return getArbitrary().sampleStream(); | ||
} finally { | ||
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null | ||
} |
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.
@SuppressWarnings("unchecked")
@Override
public Stream<T> sampleStream() {
return getArbitrary()
.filter((Predicate<T>)this.validateFilter(validOnly))
.sampleStream();
}
이렇게 처리하면 https://github.com/naver/fixture-monkey/pull/33/files#diff-b2c1b34618d723d483d2f23778d1695dd5107b38c91b4d0301c2b92e65842bf3R329 의 경우를 처리할 수 있습니다.
sample을 처리하는 메소드가 두 개 뿐이라 이렇게 처리하는 게 더 좋아보입니다.
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 을 포함해서 다른 모든 메소드에 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
아 다른 Arbitrary 들을 생성하는 메소드들도 처리하고 싶으신거군요, 이해했습니다
예외처리가 각 메소드로 분리되서 관리가 힘들 것 같은데, 더 좋은 방법을 생각해보고 없으면 이렇게 해야할 것 같습니다.
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 의 구조와 처리방식은 고민해보고 이후에 개선을 할 필요가 있을거같읍니다
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
validatorFilter를 public으로 전환하면 다음과 같이 처리가 가능한데요, 이건 어떨까요?
ArbitraryTree#result
public Arbitrary<T> result(
Supplier<Arbitrary<T>> generateArbitrary,
ArbitraryValidator<T> validator,
boolean validOnly
) {
ArbitraryValue arbitraryValue = new ArbitraryValue(generateArbitrary, validator, validOnly);
return arbitraryValue
.filter(arbitraryValue.validateFilter(validOnly));
}
filter를 밖에서 실행하면 validator, 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.
@seongahjo
네 그렇게 되면 ArbitraryValue 의 sample 에서 TooManyFilter 에러가 났을때 violation 수집을 못 합니다
이미 sampleStream 을 포함해 다른 Arbitrary 를 메소드를 호출한후 sampling 할때 violation 수집을 못하는 상태이긴합니다
그래서 아예 violation 수집을 빼버리거나 전체를 적용할수있는 구조로 개선해야될거같습니다
sample 에서 빼는것도 제공되던 기능이 없어지는거므로 0.4.0 에 진행하면좋겠군요
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
넵, 일단 이렇게 하고 더 나은 개선방안이 있을지 고민해보면 좋을 것 같습니다.