From e8ab978eb5c7c48cdb77dfbba1fa81ff71bbc10b Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Wed, 3 Dec 2025 20:44:58 +0100 Subject: [PATCH 1/4] robot-name: Redesign for testability 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`. --- .../robot-name/.docs/instructions.append.md | 9 -- .../robot-name/.meta/Cargo-example.toml | 10 -- .../practice/robot-name/.meta/config.json | 1 + .../practice/robot-name/.meta/example.rs | 64 +++++++------ exercises/practice/robot-name/Cargo.toml | 3 + exercises/practice/robot-name/src/lib.rs | 23 ++++- .../practice/robot-name/tests/robot_name.rs | 94 ++++++++++++++----- .../tests/no_underscore_prefixed_idents.rs | 1 + 8 files changed, 131 insertions(+), 74 deletions(-) delete mode 100644 exercises/practice/robot-name/.docs/instructions.append.md delete mode 100644 exercises/practice/robot-name/.meta/Cargo-example.toml diff --git a/exercises/practice/robot-name/.docs/instructions.append.md b/exercises/practice/robot-name/.docs/instructions.append.md deleted file mode 100644 index 48930b13d..000000000 --- a/exercises/practice/robot-name/.docs/instructions.append.md +++ /dev/null @@ -1,9 +0,0 @@ -# Instructions append - -## Global Mutable State - -The way the tests are setup, you will be forced to use global mutable state. -This is generally frowned-upon and considered unidiomatic in Rust. -However, it's a delibarate choice for the purpose of learning here. - -Can you think of a better API design that doesn't use global mutable state? diff --git a/exercises/practice/robot-name/.meta/Cargo-example.toml b/exercises/practice/robot-name/.meta/Cargo-example.toml deleted file mode 100644 index bceec03b5..000000000 --- a/exercises/practice/robot-name/.meta/Cargo-example.toml +++ /dev/null @@ -1,10 +0,0 @@ -[package] -name = "robot_name" -version = "0.1.0" -edition = "2024" - -# Not all libraries from crates.io are available in Exercism's test runner. -# The full list of available libraries is here: -# https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml -[dependencies] -rand = "0.3.12" diff --git a/exercises/practice/robot-name/.meta/config.json b/exercises/practice/robot-name/.meta/config.json index c5d2fa26d..2a1722971 100644 --- a/exercises/practice/robot-name/.meta/config.json +++ b/exercises/practice/robot-name/.meta/config.json @@ -22,6 +22,7 @@ "pminten", "razielgn", "rofrol", + "senekor", "spazm", "stringparser", "workingjubilee", diff --git a/exercises/practice/robot-name/.meta/example.rs b/exercises/practice/robot-name/.meta/example.rs index 487312a77..d03c6e2bc 100644 --- a/exercises/practice/robot-name/.meta/example.rs +++ b/exercises/practice/robot-name/.meta/example.rs @@ -1,52 +1,62 @@ use std::{ collections::HashSet, - sync::{LazyLock, Mutex}, + sync::{Arc, Mutex}, }; -use rand::{Rng, thread_rng}; +use rand::Rng; -static NAMES: LazyLock>> = LazyLock::new(|| Mutex::new(HashSet::new())); - -pub struct Robot { - name: String, -} - -fn generate_name() -> String { +fn generate_name(rng: &mut R, used_names: &mut HashSet) -> String { loop { let mut s = String::with_capacity(5); - static LETTERS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZ"; - static NUMBERS: &[u8] = b"0123456789"; for _ in 0..2 { - s.push(*thread_rng().choose(LETTERS).unwrap() as char); + s.push(rng.random_range('A'..='Z')); } for _ in 0..3 { - s.push(*thread_rng().choose(NUMBERS).unwrap() as char); + s.push(rng.random_range('0'..='9')); } - - if NAMES.lock().unwrap().insert(s.clone()) { + if used_names.insert(s.clone()) { return s; } } } -impl Robot { - pub fn new() -> Robot { - Robot { - name: generate_name(), +/// A `RobotFactory` is responsible for ensuring that all robots produced by +/// it have a unique name. Robots from different factories can have the same +/// name. +pub struct RobotFactory { + used_names: Arc>>, +} + +pub struct Robot { + used_names: Arc>>, + name: String, +} + +impl RobotFactory { + pub fn new() -> Self { + Self { + used_names: Arc::new(Mutex::new(HashSet::new())), } } - pub fn name(&self) -> &str { - &self.name[..] + pub fn new_robot(&mut self, rng: &mut R) -> Robot { + let mut guard = self.used_names.lock().unwrap(); + Robot { + used_names: Arc::clone(&self.used_names), + name: generate_name(rng, &mut guard), + } } +} - pub fn reset_name(&mut self) { - self.name = generate_name(); +impl Robot { + pub fn name(&self) -> &str { + &self.name } -} -impl Default for Robot { - fn default() -> Self { - Self::new() + // When a robot is reset, its factory must still ensure that there are no + // name conflicts with other robots from the same factory. + pub fn reset(&mut self, rng: &mut R) { + let mut used_names = self.used_names.lock().unwrap(); + self.name = generate_name(rng, &mut used_names); } } diff --git a/exercises/practice/robot-name/Cargo.toml b/exercises/practice/robot-name/Cargo.toml index d584df2f0..e7d759307 100644 --- a/exercises/practice/robot-name/Cargo.toml +++ b/exercises/practice/robot-name/Cargo.toml @@ -7,6 +7,9 @@ edition = "2024" # The full list of available libraries is here: # https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml [dependencies] +# We use a wild-card specifier to minimize the risk of breaking existing +# solutions in case the version in the test runner is updated. +rand = "*" [lints.clippy] new_without_default = "allow" diff --git a/exercises/practice/robot-name/src/lib.rs b/exercises/practice/robot-name/src/lib.rs index 37bf158ed..6db419e9b 100644 --- a/exercises/practice/robot-name/src/lib.rs +++ b/exercises/practice/robot-name/src/lib.rs @@ -1,15 +1,28 @@ +use rand::Rng; + +/// A `RobotFactory` is responsible for ensuring that all robots produced by +/// it have a unique name. Robots from different factories can have the same +/// name. +pub struct RobotFactory; + pub struct Robot; -impl Robot { +impl RobotFactory { pub fn new() -> Self { - todo!("Construct a new Robot struct."); + todo!("Create a new robot factory") } + pub fn new_robot(&mut self, _rng: &mut R) -> Robot { + todo!("Create a new robot with a unique name") + } +} + +impl Robot { pub fn name(&self) -> &str { - todo!("Return the reference to the robot's name."); + todo!("Return a reference to the robot's name"); } - pub fn reset_name(&mut self) { - todo!("Assign a new unique name to the robot."); + pub fn reset(&mut self, _rng: &mut R) { + todo!("Assign a new unique name to the robot"); } } diff --git a/exercises/practice/robot-name/tests/robot_name.rs b/exercises/practice/robot-name/tests/robot_name.rs index abfeadd3f..abc5d3428 100644 --- a/exercises/practice/robot-name/tests/robot_name.rs +++ b/exercises/practice/robot-name/tests/robot_name.rs @@ -1,4 +1,12 @@ -use robot_name as robot; +use std::collections::HashSet; + +use rand::SeedableRng as _; +use rand::rngs::SmallRng; +use robot_name::*; + +fn deterministic_rng() -> SmallRng { + SmallRng::seed_from_u64(0) +} fn assert_name_matches_pattern(n: &str) { assert!(n.len() == 5, "name is exactly 5 characters long"); @@ -14,46 +22,86 @@ fn assert_name_matches_pattern(n: &str) { #[test] fn name_should_match_expected_pattern() { - let r = robot::Robot::new(); - assert_name_matches_pattern(r.name()); + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let robot = factory.new_robot(&mut rng); + assert_name_matches_pattern(robot.name()); } #[test] #[ignore] -fn different_robots_have_different_names() { - let r1 = robot::Robot::new(); - let r2 = robot::Robot::new(); - assert_ne!(r1.name(), r2.name(), "Robot names should be different"); +fn factory_prevents_name_collisions() { + let mut factory = RobotFactory::new(); + let robot_1 = factory.new_robot(&mut deterministic_rng()); + let robot_2 = factory.new_robot(&mut deterministic_rng()); + assert_ne!(robot_1.name(), robot_2.name()); } #[test] #[ignore] -fn many_different_robots_have_different_names() { - use std::collections::HashSet; +fn robot_name_depends_on_rng() { + let mut rng = deterministic_rng(); + let robot_1 = RobotFactory::new().new_robot(&mut rng); + let robot_2 = RobotFactory::new().new_robot(&mut rng); + assert_ne!(robot_1.name(), robot_2.name()); +} - // In 3,529 random robot names, there is ~99.99% chance of a name collision - let vec: Vec<_> = (0..3529).map(|_| robot::Robot::new()).collect(); - let set: HashSet<_> = vec.iter().map(|robot| robot.name()).collect(); +#[test] +#[ignore] +fn robot_name_only_depends_on_rng() { + let robot_1 = RobotFactory::new().new_robot(&mut deterministic_rng()); + let robot_2 = RobotFactory::new().new_robot(&mut deterministic_rng()); + assert_eq!(robot_1.name(), robot_2.name()); +} - let number_of_collisions = vec.len() - set.len(); - assert_eq!(number_of_collisions, 0); +#[test] +#[ignore] +fn many_different_robots_have_different_names() { + // In 3,529 random robot names, there is ~99.99% chance of a name collision + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect(); + let mut set = HashSet::new(); + assert!(robots.iter().all(|robot| set.insert(robot.name()))); } #[test] #[ignore] fn new_name_should_match_expected_pattern() { - let mut r = robot::Robot::new(); - assert_name_matches_pattern(r.name()); - r.reset_name(); - assert_name_matches_pattern(r.name()); + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let mut robot = factory.new_robot(&mut rng); + assert_name_matches_pattern(robot.name()); + robot.reset(&mut rng); + assert_name_matches_pattern(robot.name()); } #[test] #[ignore] fn new_name_is_different_from_old_name() { - let mut r = robot::Robot::new(); - let n1 = r.name().to_string(); - r.reset_name(); - let n2 = r.name().to_string(); - assert_ne!(n1, n2, "Robot name should change when reset"); + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let mut robot = factory.new_robot(&mut rng); + let name_1 = robot.name().to_string(); + robot.reset(&mut rng); + let name_2 = robot.name().to_string(); + assert_ne!(name_1, name_2, "Robot name should change when reset"); +} + +#[test] +#[ignore] +fn factory_prevents_name_collision_despite_reset() { + // To keep the same probablity as the first test with many robots, we + // generate 3,529 robots and reset their names, then generate another 3,529 + // robots and check if there are collisions across these two groups. + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let mut reset_robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect(); + for robot in &mut reset_robots { + robot.reset(&mut rng); + } + let mut set = HashSet::new(); + assert!(reset_robots.iter().all(|robot| set.insert(robot.name()))); + + assert!((0..3529).all(|_| !set.contains(factory.new_robot(&mut rng).name()))); } diff --git a/rust-tooling/ci-tests/tests/no_underscore_prefixed_idents.rs b/rust-tooling/ci-tests/tests/no_underscore_prefixed_idents.rs index 7fa88b88a..57f58dcf8 100644 --- a/rust-tooling/ci-tests/tests/no_underscore_prefixed_idents.rs +++ b/rust-tooling/ci-tests/tests/no_underscore_prefixed_idents.rs @@ -56,6 +56,7 @@ static EXCEPTIONS: &[&str] = &[ "roman-numerals", // std::fmt::Formatter is not Debug "simple-linked-list", // has generics "sublist", // has generics + "robot-name", // has generics ]; fn line_is_not_a_comment(line: &&str) -> bool { From ae350c86f354eb96a407bdf6b84c7e8da7fae55c Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Thu, 4 Dec 2025 23:49:11 +0100 Subject: [PATCH 2/4] remove wild-card dependency specifier --- exercises/practice/robot-name/Cargo.toml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/exercises/practice/robot-name/Cargo.toml b/exercises/practice/robot-name/Cargo.toml index e7d759307..efa2a4786 100644 --- a/exercises/practice/robot-name/Cargo.toml +++ b/exercises/practice/robot-name/Cargo.toml @@ -7,9 +7,7 @@ edition = "2024" # The full list of available libraries is here: # https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml [dependencies] -# We use a wild-card specifier to minimize the risk of breaking existing -# solutions in case the version in the test runner is updated. -rand = "*" +rand = "0.9" [lints.clippy] new_without_default = "allow" From fe5493759af20929d519b5bb34eacfe4924f370a Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Sat, 6 Dec 2025 20:20:44 +0100 Subject: [PATCH 3/4] ensure old name is released after reset --- exercises/practice/robot-name/.meta/example.rs | 4 +++- exercises/practice/robot-name/tests/robot_name.rs | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/exercises/practice/robot-name/.meta/example.rs b/exercises/practice/robot-name/.meta/example.rs index d03c6e2bc..73e783d12 100644 --- a/exercises/practice/robot-name/.meta/example.rs +++ b/exercises/practice/robot-name/.meta/example.rs @@ -57,6 +57,8 @@ impl Robot { // name conflicts with other robots from the same factory. pub fn reset(&mut self, rng: &mut R) { let mut used_names = self.used_names.lock().unwrap(); - self.name = generate_name(rng, &mut used_names); + let new_name = generate_name(rng, &mut used_names); + used_names.remove(&self.name); + self.name = new_name } } diff --git a/exercises/practice/robot-name/tests/robot_name.rs b/exercises/practice/robot-name/tests/robot_name.rs index abc5d3428..3e14c794e 100644 --- a/exercises/practice/robot-name/tests/robot_name.rs +++ b/exercises/practice/robot-name/tests/robot_name.rs @@ -105,3 +105,16 @@ fn factory_prevents_name_collision_despite_reset() { assert!((0..3529).all(|_| !set.contains(factory.new_robot(&mut rng).name()))); } + +#[test] +#[ignore] +fn old_name_becomes_available_after_reset() { + let mut rng = deterministic_rng(); + let mut factory = RobotFactory::new(); + let mut robot = factory.new_robot(&mut rng); + let first_name = robot.name().to_string(); + robot.reset(&mut rng); // cause first name to become available again + let mut rng = deterministic_rng(); // reset rng + let second_robot = factory.new_robot(&mut rng); + assert_eq!(second_robot.name(), first_name); +} From 1e307cf1bb6cee1746265a3a24ee053197f2be93 Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Sat, 6 Dec 2025 21:05:47 +0100 Subject: [PATCH 4/4] remove remaining probabilistic tests --- .../practice/robot-name/tests/robot_name.rs | 41 ++++++------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/exercises/practice/robot-name/tests/robot_name.rs b/exercises/practice/robot-name/tests/robot_name.rs index 3e14c794e..545d79796 100644 --- a/exercises/practice/robot-name/tests/robot_name.rs +++ b/exercises/practice/robot-name/tests/robot_name.rs @@ -1,5 +1,3 @@ -use std::collections::HashSet; - use rand::SeedableRng as _; use rand::rngs::SmallRng; use robot_name::*; @@ -28,15 +26,6 @@ fn name_should_match_expected_pattern() { assert_name_matches_pattern(robot.name()); } -#[test] -#[ignore] -fn factory_prevents_name_collisions() { - let mut factory = RobotFactory::new(); - let robot_1 = factory.new_robot(&mut deterministic_rng()); - let robot_2 = factory.new_robot(&mut deterministic_rng()); - assert_ne!(robot_1.name(), robot_2.name()); -} - #[test] #[ignore] fn robot_name_depends_on_rng() { @@ -56,13 +45,11 @@ fn robot_name_only_depends_on_rng() { #[test] #[ignore] -fn many_different_robots_have_different_names() { - // In 3,529 random robot names, there is ~99.99% chance of a name collision - let mut rng = deterministic_rng(); +fn factory_prevents_name_collisions() { let mut factory = RobotFactory::new(); - let robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect(); - let mut set = HashSet::new(); - assert!(robots.iter().all(|robot| set.insert(robot.name()))); + let robot_1 = factory.new_robot(&mut deterministic_rng()); + let robot_2 = factory.new_robot(&mut deterministic_rng()); + assert_ne!(robot_1.name(), robot_2.name()); } #[test] @@ -91,19 +78,17 @@ fn new_name_is_different_from_old_name() { #[test] #[ignore] fn factory_prevents_name_collision_despite_reset() { - // To keep the same probablity as the first test with many robots, we - // generate 3,529 robots and reset their names, then generate another 3,529 - // robots and check if there are collisions across these two groups. - let mut rng = deterministic_rng(); let mut factory = RobotFactory::new(); - let mut reset_robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect(); - for robot in &mut reset_robots { - robot.reset(&mut rng); - } - let mut set = HashSet::new(); - assert!(reset_robots.iter().all(|robot| set.insert(robot.name()))); - assert!((0..3529).all(|_| !set.contains(factory.new_robot(&mut rng).name()))); + let mut rng = deterministic_rng(); + let mut robot_1 = factory.new_robot(&mut rng); + robot_1.reset(&mut rng); + + let mut rng = deterministic_rng(); + let mut robot_2 = factory.new_robot(&mut rng); + robot_2.reset(&mut rng); + + assert_ne!(robot_1.name(), robot_2.name()); } #[test]