Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix ArbitraryBuilder generated Arbitrary not affected Validator any with action #33

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Fix ArbitraryBuilder generated Arbitrary not affected Validator any with action #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
251 changes: 185 additions & 66 deletions fixture-monkey/src/main/java/com/navercorp/fixturemonkey/arbitrary/ArbitraryValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,21 @@ public ArbitraryValue(
}

@Override
public RandomGenerator<T> generator(int genSize) {
return getArbitrary().generator(genSize);
public synchronized RandomGenerator<T> generator(int genSize) {
try {
return getArbitrary().generator(genSize);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Object> asGeneric() {
return getArbitrary().asGeneric();
public synchronized Arbitrary<Object> asGeneric() {
try {
return getArbitrary().asGeneric();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
Expand All @@ -89,107 +97,180 @@ public boolean isUnique() {
}

@Override
public Optional<ExhaustiveGenerator<T>> exhaustive() {
return getArbitrary().exhaustive();
public synchronized Optional<ExhaustiveGenerator<T>> exhaustive() {
try {
return getArbitrary().exhaustive();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Optional<ExhaustiveGenerator<T>> exhaustive(long maxNumberOfSamples) {
return getArbitrary().exhaustive(maxNumberOfSamples);
public synchronized Optional<ExhaustiveGenerator<T>> exhaustive(long maxNumberOfSamples) {
try {
return getArbitrary().exhaustive(maxNumberOfSamples);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public EdgeCases<T> edgeCases() {
return getArbitrary().edgeCases();
public synchronized EdgeCases<T> edgeCases() {
try {
return getArbitrary().edgeCases();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Optional<Stream<T>> allValues() {
return getArbitrary().allValues();
public synchronized Optional<Stream<T>> allValues() {
try {
return getArbitrary().allValues();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public void forEachValue(Consumer<? super T> action) {
getArbitrary().forEachValue(action);
public synchronized void forEachValue(Consumer<? super T> action) {
try {
getArbitrary().forEachValue(action);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> filter(Predicate<T> filterPredicate) {
return getArbitrary().filter(filterPredicate);
public synchronized Arbitrary<T> filter(Predicate<T> filterPredicate) {
try {
return getArbitrary().filter(filterPredicate);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public <U> Arbitrary<U> map(Function<T, U> mapper) {
return getArbitrary().map(mapper);
public synchronized <U> Arbitrary<U> map(Function<T, U> mapper) {
try {
return getArbitrary().map(mapper);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public <U> Arbitrary<U> flatMap(Function<T, Arbitrary<U>> mapper) {
return getArbitrary().flatMap(mapper);
public synchronized <U> Arbitrary<U> flatMap(Function<T, Arbitrary<U>> mapper) {
try {
return getArbitrary().flatMap(mapper);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> injectNull(double nullProbability) {
return getArbitrary().injectNull(nullProbability);
public synchronized Arbitrary<T> injectNull(double nullProbability) {
try {
return getArbitrary().injectNull(nullProbability);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> unique() {
return getArbitrary().unique();
public synchronized Arbitrary<T> unique() {
try {
return getArbitrary().unique();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> fixGenSize(int genSize) {
return getArbitrary().fixGenSize(genSize);
public synchronized Arbitrary<T> fixGenSize(int genSize) {
try {
return getArbitrary().fixGenSize(genSize);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public ListArbitrary<T> list() {
return getArbitrary().list();
public synchronized ListArbitrary<T> list() {
try {
return getArbitrary().list();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public SetArbitrary<T> set() {
return getArbitrary().set();
public synchronized SetArbitrary<T> set() {
try {
return getArbitrary().set();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public StreamArbitrary<T> stream() {
return getArbitrary().stream();
public synchronized StreamArbitrary<T> stream() {
try {
return getArbitrary().stream();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public IteratorArbitrary<T> iterator() {
return getArbitrary().iterator();
public synchronized IteratorArbitrary<T> iterator() {
try {
return getArbitrary().iterator();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public <A> StreamableArbitrary<T, A> array(Class<A> arrayClass) {
return getArbitrary().array(arrayClass);
public synchronized <A> StreamableArbitrary<T, A> array(Class<A> arrayClass) {
try {
return getArbitrary().array(arrayClass);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Optional<T>> optional() {
return getArbitrary().optional();
public synchronized Arbitrary<Optional<T>> optional() {
try {
return getArbitrary().optional();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<List<T>> collect(Predicate<List<T>> until) {
return getArbitrary().collect(until);
public synchronized Arbitrary<List<T>> collect(Predicate<List<T>> until) {
try {
return getArbitrary().collect(until);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@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
}
Comment on lines 261 to +267
Copy link
Contributor

@seongahjo seongahjo Oct 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhyeon-lee

@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을 처리하는 메소드가 두 개 뿐이라 이렇게 처리하는 게 더 좋아보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 결국 sampleStream 을 포함해서 다른 모든 메소드에 filter 처리후 반환하도록 해줘야합니다

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhyeon-lee
아 다른 Arbitrary 들을 생성하는 메소드들도 처리하고 싶으신거군요, 이해했습니다

예외처리가 각 메소드로 분리되서 관리가 힘들 것 같은데, 더 좋은 방법을 생각해보고 없으면 이렇게 해야할 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 현재의 ArbitraryValue 의 구조와 처리방식은 고민해보고 이후에 개선을 할 필요가 있을거같읍니다

Copy link
Contributor

@seongahjo seongahjo Oct 2, 2021 •
edited
Loading

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를 필드로 안가져도 되겠군요.

Copy link
Collaborator Author

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 에 진행하면좋겠군요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhyeon-lee
넵, 일단 이렇게 하고 더 나은 개선방안이 있을지 고민해보면 좋을 것 같습니다.

}

@SuppressWarnings("unchecked")
@Override
public T sample() {
public synchronized T sample() {
try {
return getArbitrary()
.filter((Predicate<T>)this.validateFilter(validOnly))
.sample();
return getArbitrary().sample();
} catch (TooManyFilterMissesException ex) {
StringBuilder builder = new StringBuilder();
this.violations.values().forEach(violation -> builder
Expand All @@ -209,53 +290,91 @@ public T sample() {
}

@Override
public Arbitrary<T> injectDuplicates(double duplicateProbability) {
return getArbitrary().injectDuplicates(duplicateProbability);
public synchronized Arbitrary<T> injectDuplicates(double duplicateProbability) {
try {
return getArbitrary().injectDuplicates(duplicateProbability);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Tuple1<T>> tuple1() {
return getArbitrary().tuple1();
public synchronized Arbitrary<Tuple1<T>> tuple1() {
try {
return getArbitrary().tuple1();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Tuple2<T, T>> tuple2() {
return getArbitrary().tuple2();
public synchronized Arbitrary<Tuple2<T, T>> tuple2() {
try {
return getArbitrary().tuple2();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Tuple3<T, T, T>> tuple3() {
return getArbitrary().tuple3();
public synchronized Arbitrary<Tuple3<T, T, T>> tuple3() {
try {
return getArbitrary().tuple3();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Tuple4<T, T, T, T>> tuple4() {
return getArbitrary().tuple4();
public synchronized Arbitrary<Tuple4<T, T, T, T>> tuple4() {
try {
return getArbitrary().tuple4();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<Tuple5<T, T, T, T, T>> tuple5() {
return getArbitrary().tuple5();
public synchronized Arbitrary<Tuple5<T, T, T, T, T>> tuple5() {
try {
return getArbitrary().tuple5();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> ignoreException(Class<? extends Throwable> exceptionType) {
return getArbitrary().ignoreException(exceptionType);
public synchronized Arbitrary<T> ignoreException(Class<? extends Throwable> exceptionType) {
try {
return getArbitrary().ignoreException(exceptionType);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> dontShrink() {
return getArbitrary().dontShrink();
public synchronized Arbitrary<T> dontShrink() {
try {
return getArbitrary().dontShrink();
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@Override
public Arbitrary<T> edgeCases(Consumer<Config<T>> configurator) {
return getArbitrary().edgeCases(configurator);
public synchronized Arbitrary<T> edgeCases(Consumer<Config<T>> configurator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

드디어 synchronized가 필요한 경우가 발생했나요?

Copy link
Collaborator Author

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 에 병렬 호출을 하지 않을거 같지만
사용자의 선택범위이니 동시성 방어를 해주는게 좋을거 같습니다

try {
return getArbitrary().edgeCases(configurator);
} finally {
this.arbitrary = null; // in order to getting new value whenever sampling, set arbitrary as null
}
}

@SuppressWarnings("unchecked")
private synchronized Arbitrary<T> getArbitrary() {
if (this.arbitrary == null) {
this.arbitrary = generateArbitrary.get();
this.arbitrary = generateArbitrary.get()
.filter((Predicate<T>)this.validateFilter(validOnly));
}
return this.arbitrary;
}
Expand Down
Loading