Skip to content

dominoes: make test code more idiomatic#2127

Merged
senekor merged 1 commit intomainfrom
senekor/smxkzywyqpnu
Feb 15, 2026
Merged

dominoes: make test code more idiomatic#2127
senekor merged 1 commit intomainfrom
senekor/smxkzywyqpnu

Conversation

@senekor
Copy link
Contributor

@senekor senekor commented Feb 13, 2026

I've had a mentee copy the "normalize" function from the test code. I didn't notice at first and explained it's unidiomatic to use a match statement only for the guards, because it's just a weird way of writing an if-else-statement.

This version isn't an if-else-statement (which would've been fine too), but I find it the most readable. I have confirmed that the generated assemply is essentially the same, i.e. this doesn't generate two comparison instructions.

[no important files changed]

I've had a mentee copy the "normalize" function from the test code. I
didn't notice at first and explained it's unidiomatic to use a match
statement only for the guards, because it's just a weird way of writing
an if-else-statement.

This version isn't an if-else-statement (which would've been fine too),
but I find it the most readable. I have confirmed that the generated
assemply is essentially the same, i.e. this doesn't generate _two_
comparison instructions.
@github-actions
Copy link
Contributor

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 senekor requested a review from ellnix February 13, 2026 00:05
Copy link
Contributor

@ellnix ellnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, unfortunate to find out you were calling out our own code 😆

@ellnix
Copy link
Contributor

ellnix commented Feb 15, 2026

As a side note, can we discuss what's going on with the example.rs of this exercise? It seems to be working by accident, there is no backtracking logic whatsoever.

If you take the backtrack test and simply put the first domino at the end in the input, the solution fails:

#[test]
#[ignore]
fn need_backtrack() {
    // (2, 4) used to be at the end, which is used as the first domino in example.rs, causing it to pass this test by accident
    // The solution simply grabs the first domino it can find, starting from the smallest values, and since it starts with (4, 2) it doesn't run into the problem of hitting (2, 3) before (2, 4) and needing to backtrack
    let input = &[(2, 3), (3, 1), (2, 4), (2, 4), (1, 2)];
    let output = dominoes::chain(input);
    assert!(output.is_some());
    assert_correct(input, output.unwrap());
}

@senekor
Copy link
Contributor Author

senekor commented Feb 15, 2026

Huh, strange. I guess we can replace the example with a sound solution. I considered if the tests need to be improved to cover this somehow. But I don't think there's value in adding tests against strange and random edge cases like "the solution reads the first domino from the back, but the rest in order".

@senekor senekor merged commit 198d36d into main Feb 15, 2026
13 checks passed
@senekor senekor deleted the senekor/smxkzywyqpnu branch February 15, 2026 17:57
@ellnix
Copy link
Contributor

ellnix commented Feb 15, 2026

@senekor It's stranger than that: it doesn't even look at the dominoes in order. Instead, it effectively searches for a matching domino starting from the lowest number on the insignificant side.

I'm not sure reading the first domino from the end is strange, I imagine most devs would reach for some pop()-like function in their particular language.

senekor added a commit that referenced this pull request Feb 15, 2026
It was discovered that the previous example solution doesn't perform any
backtracking and only passes the relevant test accidentally:
#2127 (comment)
senekor added a commit that referenced this pull request Feb 15, 2026
It was discovered that the previous example solution doesn't perform any
backtracking and only passes the relevant test accidentally:
#2127 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments