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

Suggestion: Add a warning to the README that default random seed is now fixed #38

Open
liiight opened this issue Jan 15, 2019 · 2 comments

Comments

@liiight
Copy link

liiight commented Jan 15, 2019

After I installed this plugin I began seeing bizarre issues in my tests, like API returning 409 conflict claiming duplicate object creations. It took me a second and some time digging into the logs to figure out that my previous randomness that was in-charge of generating random object identifiers, is no longer as random as I though.

I then connected the dots and figured out that it was the doing of this plugin. I tried changing the scope of the bucket to the minimal level, class, but while the number of these issues dramatically decreased, they still occurred, as I have tests within the same class that require separate random seeds.

I am by no means "blaming" you for this as this plugin worked as it is designed and it is I who didn't fully read and/or understood the implications of using it. I am grateful for its creation and I'm sure many people get high value from it. I do have some suggestions in order to avoid lazy people like me in the future:

  1. Add a noticeable warning that this plugin manipulates the random seed and that it may disturb tests that rely on the random module.
  2. If I am not mistaken, the default random bucket scope is global. Perhaps it would be wise to either set it to the minimum scope by default or even not have a default at all, forcing the user to choose a scope so he'll have some understanding at least on the implications.

Again, I am very grateful for this plugin and I hope my suggestion are taken in the spirit they are written.

@jbasko
Copy link
Collaborator

jbasko commented Jan 15, 2019

Hi, thanks for the message.

By default, there's no randomisation anymore unless you enable it in one of three ways via CLI: --random-order (which will set module bucket type), --random-order-seed [seed] (which will set module bucket type unless you also specify --random-order-bucket=[bucket_type]), or --random-order-bucket=[bucket_type]. If you're referring to "by-default-once-it's-enabled" then it's module.

Re:warning, thanks for the suggestion, will add it the next time when I touch this.

I think the approach we have here is incompatible with one using custom seeds in their own tests. Just thinking loud, maybe we should do random.getstate() before every test case and then set it back after the test case with random.setstate().

@xmo-odoo
Copy link

xmo-odoo commented Feb 27, 2019

@jbasko couldn't the plugin just use its own random.Random() instance, independent from the global one? the functions from the random module are pretty much just proxies to the corresponding method of a global Random instance (random._inst)

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

No branches or pull requests

3 participants