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

Support for 'H' as a randomization parameter #47

Closed
wants to merge 1 commit into from

Conversation

mhupman
Copy link

@mhupman mhupman commented Feb 27, 2016

I think this will work fine, but not sure if it needs unit tests (or what the best way to test it would be). It seems like the only thing worth testing is whether or not the range passed to Intn() gets incremented so it becomes inclusive - thoughts?

@robfig
Copy link
Owner

robfig commented Mar 28, 2016

I think the way to test it would be to replace the "random" variable with a random source with a specified seed. Tests are worth it, IMO

@mhupman
Copy link
Author

mhupman commented Mar 28, 2016

So, I did get pretty far down the testing path and ended up abandoning it when I ran into some issues:

  • I can construct a rand.Source to ensure the same seed is used during tests, but the numbers returned by rand.Intn() would still be a pseudo-random. I could hardcode tests assuming same stream comes back every time, but that seems brittle?
  • In the end, if I controlled the random values, it seems like all I would be testing is that the range I'm passing to the Intn function is correct (accounting for half-open range, for example).

Anyway, I struggled with whether that was worth testing or how contrived it would be. I got as far as mocking rand.Intn() and then validating that given a range {0, 60}, the rand function received {0, 59}. That seemed pretty silly though, so I dropped it.

@robfig
Copy link
Owner

robfig commented Mar 28, 2016

I'm quite sure that you are guaranteed that the pseudo-random sequence will not change given the same seed, across different runs of the program. e.g. http://play.golang.org/p/HICPQ5ay90

If you want to control the random numbers provided, declaring a

type rand interface { Int31() int32 } 

doesn't seem so bad either.

I do think adding a new feature like this needs some sort of test coverage and documentation update

@robfig
Copy link
Owner

robfig commented Aug 9, 2016

Any interest in continuing this PR?

@crazy-max
Copy link

@robfig @mhupman Maybe I can take over this PR?

@mhupman
Copy link
Author

mhupman commented Sep 2, 2019

@crazy-max sounds good!

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.

3 participants