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

Increase default randomness for integer generators #59

Merged
merged 7 commits into from Jan 22, 2020

Conversation

AdrianLC
Copy link
Contributor

I think 10000 is just too low.
We have been having flakyness in our tests with UNIQUE constraints on Integer columns because of conflicting values we this generator.

AdrianLC and others added 2 commits January 21, 2020 12:00
I think 10000 is just too low. 
Wwe have been having flakyness on our tests with UNIQUE constraints on Integer columns because of conflicting values we this generator.
@AdrianLC AdrianLC changed the title Increase default MAX_INT random generator Increase default randomness for integer generators Jan 21, 2020
Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

The PR looks great! I just left a minor comment to make bakery more consistent with this base operations class.

model_bakery/generators.py Show resolved Hide resolved
model_bakery/generators.py Show resolved Hide resolved
Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks! The code looks great now. One last thing, can also update the changelog with a short description of this new behavior?

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks @AdrianLC for your PR! It's great now and I'm just waiting for travis to finish the build and then I'll merge it 👍

@berinhard berinhard merged commit a73a974 into model-bakers:master Jan 22, 2020
@amureki
Copy link
Collaborator

amureki commented Jan 23, 2020

Hey fellows!
I wonder, if increasing the default of MAX_INT from 10.000 to 100.000.000.000 brings unnecessary overheads to all testing systems that are using model_bakery only to cover one requested use case (won't gen_integer(1, 100000000000) work in that case?).

I did not do big testing rounds yet, but noticed in a couple of projects that I have to adapt the code, for example, I am generating values for some external validation that does not accept integers with the length longer than 8, which was working before with just gen_integer(). Now I would need to use my own values, so explicitly write something like:

MIN_INT = 1
MAX_INT = 10000

def gen_integer(min_int=-MAX_INT, max_int=MAX_INT):
    return randint(min_int, max_int)

Which is literally duplicating our code in model_bakery.random_gen.gen_integer with different constants. And gen_float() is not even supporting this option, it just uses gen_integer() with defaults, so I have to rewrite it as well. I do not know if this is a correct way to go. I am also not sure, if allowing users to set their defaults in settings or somewhere else would be the best option.
What do you think @berinhard ?

@berinhard
Copy link
Member

Thanks for bringing this up @amureki. I tested this patch before merging it in a few projects from the company I work for, and I couldn't notice any performance or error being raised.

I didn't think about the settings thing during my review and you're 100000% right about this being a best option. We give the user the ability to configure the numbers range without the need of introducing more repeated code. I'm a +1 for this refactoring.

@amureki and @anapaulagomes I'm sorry for merging this one before getting your thoughts. It's totally on me and I'll be more careful to not repeat this in the future again. @amureki would you mind opening an issue addressing this conversation so we can go on with the settings solution?

@amureki
Copy link
Collaborator

amureki commented Jan 27, 2020

I've added an issue here: #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants