3. Random ID generation #3
Conversation
ghost
ghost
changed the title
|
|
||
| // IDPrefix returns a random identifier with a given prefix. The prefix length does not count towards the | ||
| // length. | ||
| func IDPrefix(prefix string, length uint, characterSpace CharacterRange) string { |
There was a problem hiding this comment.
This seems like extra cruft that's overkill.
| // randomness. This function guarantees that when queried in order, the values are always the same as long as the name | ||
| // of the test doesn't change. | ||
| func DeterministicID(t *testing.T, length uint, characterSpace CharacterRange) string { | ||
| return IDFromSource(DeterministicSource(t), length, characterSpace) |
There was a problem hiding this comment.
Do we have existing scenarios that this would be used for?
|
|
||
| // DeterministicSource produces a rand.Rand that is deterministic based on the provided test name. It will always | ||
| // supply the same values as long as the test name doesn't change. | ||
| func DeterministicSource(t *testing.T) *rand.Rand { |
There was a problem hiding this comment.
I'm honestly not a fan of this function and don't see where it would be useful.
There was a problem hiding this comment.
I think it could be useful for some tests, but it may lead to confusion when you change the function name. The test will not pass and it could be hard to say why from the first glance.
Also, code under test should provide consistent results so it may be not as universal as we think. I had hard times changing all the iterations over maps in my mock implementation for tofu test.
Do we have a real-world example outside of this PR?
There was a problem hiding this comment.
The main use case is that if something fails in CI with a random number, it can be harder to reproduce locally if said random number wasn't deterministic. The certificate generation PR (#5) makes use of this function. (This still needs some work to be fully deterministic.)
|
Overall, I think that the charset idea is useful, but a lot of the other stuff introduced is overkill. |
|
@cam72cam much of this is based on existing randomness needs in OpenTofu, see the corresponding refactoring PR opentofu/opentofu#1731 |
|
Looking at the linked PR does not change my above comments. The deterministic from t.Name is not used and IMO not a good idea. The Prefix case is used, but does not buy us anything for 2 simple use cases. |
abstractionfactory
force-pushed
the
2-test-context
branch
3 times, most recently
from
f9cbaec to
579f79e
Compare
abstractionfactory
force-pushed
the
3-test-random
branch
from
ce16151 to
8887fb5
Compare
abstractionfactory
force-pushed
the
2-test-context
branch
from
579f79e to
a2b3c43
Compare
Note: this commit contains code based on OpenTofu code and therefore, changes the license headers Signed-off-by: Janos Bonic <[email protected]> Co-authored-by: AbstractionFactory <[email protected]>
Signed-off-by: Janos <[email protected]> Co-authored-by: AbstractionFactory <[email protected]>
Signed-off-by: AbstractionFactory <[email protected]>
abstractionfactory
force-pushed
the
3-test-random
branch
from
cfa409d to
7346b4c
Compare
This PR is based on #2 and adds the functions for random number generation from opentofu/opentofu#1731