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.

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

Repeatable random object on the same seed #413

Closed

Conversation

seregamorph
Copy link
Contributor

The problem: EasyRandom object initialized with the same seed parameter should generate same POJO instances. This contract came from java.util.Random is respected by most of Randomizer implementations. But there is still at least two places that generate new Random() object without seed. As a result the generated POJOs are different.

The context: in my project I validate equals/hashCode/toString implementations for the POJOs that have back references. I generate random beans by EasyRandom instance initialized with same seed value and then call equals/hashCode for such pairs, separate test calls toString(). Expected result: equals=true, hashCode should also equal, all three methods do not throw StackOverflowError (if POJO developer is not careful with equals/hashCode/toString exclude (lombok), the call will lead to infinite nested traversals).

Possible workarounds: for now it can be solved by setting EasyRandomParameters().objectPoolSize(1). Because if the populatedBeans list collection size is single element, there is no options to randomly choose other than first.

Proposed solution: share Random object in RandomizationContext.

TDD: RepeatableRandomTest has test that should fail with old implementation. It's the minimal model that reproduces the issue.

@seregamorph
Copy link
Contributor Author

@benas any ideas, objections, or maybe additional clarifications required?

@fmbenhassine
Copy link
Member

Hi @seregamorph , thank you for the PR! I did not look at it yet, I have very little spare time during these days which I'm using for another project. I will take a look asap and let you know. Thank you.

@seregamorph
Copy link
Contributor Author

@benas any chance it will be reviewed?

@fmbenhassine
Copy link
Member

Good catch! With the same seed, we should end-up with the same generated objects (assuming equals/hashcode are correctly implemented). If this is not the case, I consider it as a bug in Easy Random. I will test your PR in details and get back to you if I need more clarification. Thank you!

@fmbenhassine fmbenhassine added this to the 4.3.0 milestone Oct 9, 2020
@seregamorph
Copy link
Contributor Author

With the same seed, we should end-up with the same generated objects

Exactly, this PR addresses an issue with it.

*
* @author Mahmoud Ben Hassine ([email protected])
*/
public final class CollectionUtils {
Copy link
Member

Choose a reason for hiding this comment

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

Even if this is documented to be intended only for internal use, I do not remove public APIs without notice. Please put this back and mark it as deprecated (See how DateUtils has been deprecated in the same way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted and marked deprecated

return list.get(nextInt(0, list.size()));
}

private static int nextInt(int startInclusive, int endExclusive) {
Copy link
Member

Choose a reason for hiding this comment

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

This is actually duplicated in RandomizationContext. Both methods seems to be called always with startInclusive = 0, which mean this is no different than Random.nextInt(bound), so we can get rid of these methods and use the built-in method instead (I will show how in other comments).

* @param <T> the type of elements in the list
* @return a random element from the list or null if the list is empty
*/
<T> T randomElementOf(List<T> list);
Copy link
Member

Choose a reason for hiding this comment

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

In addition to being a breaking change, this does not belong to the contract of RandomizerContext. We can manage to have a random element from a list where this is used (ie in FieldPopulator and ObjenesisObjectFactory) without this method, see other comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -128,7 +127,7 @@ private Object generateRandomValue(final Field field, final RandomizationContext
value = mapPopulator.getRandomMap(field, context);
} else {
if (context.getParameters().isScanClasspathForConcreteTypes() && isAbstract(fieldType) && !isEnumType(fieldType) /*enums can be abstract, but can not inherit*/) {
Class<?> randomConcreteSubType = randomElementOf(filterSameParameterizedTypes(getPublicConcreteSubTypesOf(fieldType), fieldGenericType));
Class<?> randomConcreteSubType = context.randomElementOf(filterSameParameterizedTypes(getPublicConcreteSubTypesOf(fieldType), fieldGenericType));
Copy link
Member

@fmbenhassine fmbenhassine Oct 31, 2020

Choose a reason for hiding this comment

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

FieldPopulator has a reference to an EasyRandom instance (which is initialized with the same seed), so this can be replaced with:

List<Class<?>> parameterizedTypes = filterSameParameterizedTypes(getPublicConcreteSubTypesOf(fieldType), fieldGenericType);
if (parameterizedTypes.isEmpty()) {
    throw new ObjectCreationException("Unable to find a matching concrete subtype of type: " + fieldType);
} else {
    Class<?> randomConcreteSubType = parameterizedTypes.get(easyRandom.nextInt(parameterizedTypes.size()));
    value = easyRandom.doPopulateBean(randomConcreteSubType, context);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for exact code sample. done

@@ -47,7 +46,7 @@
@Override
public <T> T createInstance(Class<T> type, RandomizerContext context) {
if (context.getParameters().isScanClasspathForConcreteTypes() && isAbstract(type)) {
Class<?> randomConcreteSubType = randomElementOf(getPublicConcreteSubTypesOf((type)));
Class<?> randomConcreteSubType = context.randomElementOf(getPublicConcreteSubTypesOf((type)));
Copy link
Member

@fmbenhassine fmbenhassine Oct 31, 2020

Choose a reason for hiding this comment

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

We can use a Random instance here and initialize it (only once) with the seed from the context:

    private Random random;

    @Override
    public <T> T createInstance(Class<T> type, RandomizerContext context) {
        if (random == null) {
            random = new Random(context.getParameters().getSeed());
        }
        if (context.getParameters().isScanClasspathForConcreteTypes() && isAbstract(type)) {
            List<Class<?>> publicConcreteSubTypes = getPublicConcreteSubTypesOf(type);
            if (publicConcreteSubTypes.isEmpty()) {
                throw new InstantiationError("Unable to find a matching concrete subtype of type: " + type + " in the classpath");
            } else {
                Class<?> randomConcreteSubType = publicConcreteSubTypes.get(random.nextInt(publicConcreteSubTypes.size()));
                return (T) createNewInstance(randomConcreteSubType);
            }
        } else {
            try {
                return createNewInstance(type);
            } catch (Error e) {
                throw new ObjectCreationException("Unable to create an instance of type: " + type, e);
            }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -105,7 +108,7 @@ boolean hasExceededRandomizationDepth() {
}

private int nextInt(int startInclusive, int endExclusive) {
Copy link
Member

@fmbenhassine fmbenhassine Oct 31, 2020

Choose a reason for hiding this comment

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

This method can be removed and the line where it is called can be replaced with:

int randomIndex = actualPoolSize > 1 ? random.nextInt(actualPoolSize) : 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -148,4 +151,12 @@ public Object getRootObject() {
public EasyRandomParameters getParameters() {
return parameters;
}

@Override
public <T> T randomElementOf(List<T> list) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed as explained below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@fmbenhassine
Copy link
Member

Perfect! Thank you for these updates. Rebased, squashed and merged as 15e5a82. Thank you for your contribution 👍

@seregamorph seregamorph deleted the feature/repeatable-random branch November 2, 2020 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants