robot-name: Redesign for testability #2114
Conversation
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
senekor
force-pushed
the
senekor/ymtplounsqqn
branch
from
5dc78ed to
eef4709
Compare
The previous exercise design had two fundamental flaws wrt. testability: - The source of randomness was not injected by the tests. It was expected that users would initialize an RNG on their own. This lead to users simply not using any randomness at all. - The fact that names were required to be _globally_ unique meant that there was no way to test for deterministic behavior, given a PRNG with a stable seed. These two problems are solved by injecting an RNG from the tests and by narrowing the requirement of unique names to a single "robot factory". Motivated by this discussion on the forum: https://forum.exercism.org/t/rust-track-check-predictability-of-names-in-robot-name-exercise/20077 The function `RobotFactory::new_robot` takes a `&mut self`, even though it could take a `&self` as well. `&self` would work, because the robot factory needs shared ownership & interior mutability anyway to track robot names generated with `Robot::reset`. However, that would slightly interfere with the TTD flow of this exercise. If `RobotFactory::new_robot` takes a `&self`, users will be forced to use interior mutability right away, even though it's unclear why that is even necessary until they get around to implementing `Robot::reset`.
senekor
force-pushed
the
senekor/ymtplounsqqn
branch
from
eef4709 to
e8ab978
Compare
|
I mentioned this on the forum, but I'll repeat it here for reference: This redesign deliberately breaks all existing solutions, because at least the top two community solutions don't use any randomness. Therefore, making a clean slate of the community solutions is actually benefitial. |
mk-mxp
left a comment
mk-mxp
left a comment
There was a problem hiding this comment.
I have no idea, if that is good Rust...anyways, I think I understand enough to see the randomness issue solved.
The previous exercise design had two fundamental flaws wrt. testability:
The source of randomness was not injected by the tests. It was expected that users would initialize an RNG on their own. This lead to users simply not using any randomness at all.
The fact that names were required to be globally unique meant that there was no way to test for deterministic behavior, given a PRNG with a stable seed.
These two problems are solved by injecting an RNG from the tests and by narrowing the requirement of unique names to a single "robot factory".
Motivated by this discussion on the forum:
https://forum.exercism.org/t/rust-track-check-predictability-of-names-in-robot-name-exercise/20077
The function
RobotFactory::new_robottakes a&mut self, even though it could take a&selfas well.&selfwould work, because the robot factory needs shared ownership & interior mutability anyway to track robot names generated withRobot::reset. However, that would slightly interfere with the TTD flow of this exercise. IfRobotFactory::new_robottakes a&self, users will be forced to use interior mutability right away, even though it's unclear why that is even necessary until they get around to implementingRobot::reset.