1. Holger Krekel
  2. tox
  3. Pull requests

Pull requests

#73 Declined

Issue #125: implement randomseed

  1. Chris Jerdonek

This implements the randomseed option described in issue #125. Tests and documentation are included.

  • Issues #125: add a {randomseed} substitution string on hold

Comments (5)

  1. Holger Krekel repo owner

    Apart from some inline comments, i have two more points:

    • if we allow overriding set randomseed=value in the ini-file, it should be per-env as there might even be a case where a particular environment should always start with a particular one. I am skeptical we need an ini-setting at all, though. Any real-life use case?

    • tox should have a default setting of PYTHONHASHSEED=0 in order to make test runs repeatable without effort. It should be possible to override it with setenv PYTHONHASHSEED={random}. Reason: having e.g. a CI run fail, and then a local tox run fail differently, is bad. Eliminating as many factors as possible that prevent re-producing a problem is a goal with tox. Conciously setting to a random value is fine, though.

  2. Chris Jerdonek author

    Thanks for the comments. I will address them. Can I commit the changes on top, or should I make a new pull request with a "cleaner" history?

    • Regarding the ini file, if randomseed needed to be a particular value in a certain environment, couldn't the config section be rewritten so as not to use randomseed (e.g. PYTHONHASHSEED=100 instead of PYTHONHASHSEED={randomseed})? I can see that overriding could let things be more DRY though (so that rewriting would not be required).

    • Related to the above, I had a similar question about whether there would ever be a need for more than one random value per tox run (e.g. {randomseed:1} and {randomseed:2}). In either case, my thinking was that enhancements like these could be addressed later on as use real cases arise.

    • I'm not so sure about changing PYTHONHASHSEED by default. Since hash randomization is enabled by default in Python 3.3 (including in production settings), it seems better to keep that so as not to restrict the surface area tested. Similarly, keeping the default promotes more robust tests. I believe the issue is moot in earlier Python versions, because there hash randomization is disabled by default (which is equivalent to setting PYTHONHASHSEED=0). By the way, is it true that when hash randomization is disabled, hashing behaves the same even across systems and platforms, and not just on the same system from run to run?

    • To allow reproducibility in Python 3.3 by default while still testing all hash seeds, what if (at least in Python 3.3+) tox defaulted to setting PYTHONHASHSEED to an explicit random value and then displayed that value in the test output (perhaps only on failure). If that were done, perhaps the feature should be changed from the more general use case of randomseed to the more specific hashseed.

  3. Chris Jerdonek author

    By the way, is it true that when hash randomization is disabled, hashing behaves the same even across systems and platforms, and not just on the same system from run to run?

    FYI, to get more information, I just tried asking this question on the testing-in-python list.

  4. Holger Krekel repo owner

    I know that pip, for example, sets PYTHONHASHSEED=0 in order to have pytest-xdist (distributed testing) work. It's unclear if this is an issue with the plugin or with the pip test suite, though. Setting it to a uniform random value within a testenv should help here, and then showing the random value in the output. This has the advantage that one can then use it with -randomseed=... to try to repeat the failure while making sure it's not hash-seed related. So i am fine with your suggestion, if i understood it correctly.

    As to the ini-setting: i generally want to have new ini-settings be contained in testenvs, unless there is a very good reason not to. Note that the [testenv] section contains default settings for all test envs. There is thus seldomly a need for "totally global" settings. As the randomseed is only needed while processing an testenv it should be held as a testenv settings, i think.

    It would be best i think if you opened a new PR once you have the changes ready.

    thanks for the work!

  5. Chris Jerdonek author

    Yes, you understood my suggestion of setting PYTHONHASHSEED to a uniform random value. Ah, okay, I didn't get before that you were suggesting a testenv setting versus a totally global one. I will update the patch accordingly. Thanks for your prompt and valuable feedback!