Add uuid method #9
Conversation
spec/Random.Spec.savi
Outdated
| @@ -85,3 +85,15 @@ | |||
| total U32 = 0 | |||
| count.times -> (total += random.unbiased_u32_less_than(52)) | |||
| assert: (total.f32 / count.f32) == 25.514451980590820 | |||
|
|
|||
| :it "generates UUIDs version 4, variant 1" | |||
| random = _FakeRandom6432.new | |||
There was a problem hiding this comment.
@jemc I'm pulling from _FakeRandom6432 but I would actually love to test this with SecureRandom, too. How could we go about this?
There was a problem hiding this comment.
@tonchis
hmmm, I don't think uuid belongs to be part of the Random library. Think about uuid v4 (timestamp).
To me, a Random library has to deal with generating random values and guarantee certain distributions, which is a very very complex topic on it's own. UUID is a specification (RFC 4122) which deserves it's own library and test suit.
There was a problem hiding this comment.
@mneumann It's possible that adding a UUID library would be useful, but generating a random UUID is so common and so simple (small in scope to implement) that I think it fits nicely here in Random. Along with other potential things like generating a random base64 or hex string of a certain number of bytes. Essentially Random.uuid is just hex of a certain length with some dash characters in the middle. So I think it all fits together nicely, and also matches what you might expect to find if you're familiar with Ruby/Crystal.
@tonchis - I'm curious to hear more about why you want to test SecureRandom here - what kinds of bugs are you hoping such tests would detect?
If anything it might be more fruitful to test DeterministicRandom instead, since you could give it a certain seed value, and test that the exact same UUID string (or series of UUID strings) was always produced from that seed. SecureRandom is harder to use effectively in tests because it is not deterministic.
However, it's definitely possible to use it here. And since you asked how you would go about doing so, I can answer that here:
If you look in the manifest for Random, you can see that the test suite already pulls in DeterministicRandom as a dependency here.
You could pull in SecureRandom as a dependency in the same way, using a command like savi deps add SecureRandom --for spec, which should modify the manifest file and download the dependency. After doing so, it would automatically be available and in scope to refer to in your tests.
There was a problem hiding this comment.
@jemc I was thinking of testing with SecureRandom not precisely because of bugs, but because this way we'll have documenting tests with what will likely be the most common use case for .uuid.
However, I like the idea of being able to test the output with DeterministcRandom to showcase the uuid.
There was a problem hiding this comment.
@jemc I got your point. Makes sense to have it easily available as Random.uuid.
tonchis
force-pushed
the
enhance/add-v4-uuid-method
branch
from
16c0339 to
ff0f759
Compare
| assert: version == '4' | ||
| assert: (variant == '8' || variant == '9' || variant == 'a' || variant == 'b') |
There was a problem hiding this comment.
I actually didn't know about this! I assumed it was pure random data for the entire string in Ruby's SecureRandom.uuid but I see now that I was wrong.
Today I Learned.
This PR adds a
.uuidmethod for the Random trait. It will be available for all random implementations, particularly interesting for theSecureRandomlibrary.