robot-name: Redesign for testability

[senekor]

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.

[github-actions]

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.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

[senekor]

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]

I have no idea, if that is good Rust...anyways, I think I understand enough to see the randomness issue solved.

[ellnix]

Not that this is the only metric, but this was very fun to solve 😄

I think this redesign is excellent, there is nothing I can think of that you are not accounting for.

The one change I could see being interesting is to ensure that names are released to be used again when a robot's name is reset. With the current name pattern there are so many possible names that it's easy to find a non-conflicting name in every test, having a rotating set of names could be an interest twist.

Although of course it would also make sense for the problem to ensure that once a name is assigned once it is never assigned again, since that is what you would typically want if this was a real-life system.

This is all an aside though, not directly relevant to the goal of this PR, so this looks very good to me.

[senekor]

The one change I could see being interesting is to ensure that names are released to be used again when a robot's name is reset.

That's a great idea. The instructions state:

Your solution must ensure that every existing robot has a unique name.

(ephasis mine.) I would say your idea matches these instructions from problem-specifications perfectly.

And in combination with the randomness injection, this was quite easy to test for!

• make a robot
• reset its name
• make a second robot with the same RNG as the first one
• expect second robot's name to equal the first name of the first robot

[senekor]

I'm wonder if we should get rid of the probabilistic tests entirely. I'm not sure what value the add at this point.

[senekor]

Okay, I tweaked the tests a little bit more...

I deleted many_different_robots_have_different_names, because collision avoidance is already covered by factory_prevents_name_collisions. Since we control the RNG, there's no point in testing a larger number of robots.

I changed the test factory_prevents_name_collision_despite_reset to not be probabilistic.

I also moved the test factory_prevents_name_collisions a little further down, I think the test order makes more sense like this from a TDD perspective.

[ellnix]

I deleted many_different_robots_have_different_names, because collision avoidance is already covered by factory_prevents_name_collisions. Since we control the RNG, there's no point in testing a larger number of robots.

I agree, the student's solution is actually entirely deterministic, if you consider Rng to be a black box.

This is ready to be shipped from my perspective, but I understand if you want to wait for a review from someone from exercism's maintainers.

[senekor]

This is ready to be shipped from my perspective

Great, thanks. I'll let the PR stew for a while longer, if only in case one of us suddenly comes up with a new edge case in the next couple of days.