exercism / elixir

Exercism exercises in Elixir.
https://exercism.org/tracks/elixir
MIT License
625 stars 398 forks source link

Yacht: Reorder 3+2 full house test case in order to lower the chance of passing it coincidentally #1484

Closed esimonov closed 5 months ago

esimonov commented 5 months ago

The existing versions of "Full house two small, three big" and "Full house three small, two big" test cases share a similarity which may help pass them by chance. Namely, in both cases the group of two is comprised of the element which is a head of the list. This causes the following "solution" to be accepted:

def score(:full_house, dice) do
    case Enum.split_with(dice, fn x -> x == hd(dice) end) do
      {[a, a], [b, b, b]} -> 2*a + 3*b
      _ -> 0
    end
end

The present PR makes a tiny change in the 3+2 test case that will require to also consider an alternative grouping:

def score(:full_house, dice) do
    case Enum.split_with(dice, fn x -> x == hd(dice) end) do
      {[a, a], [b, b, b]} -> 2*a + 3*b
      {[a, a, a], [b, b]} -> 3*a + 2*b # <-- This one.
      _ -> 0
    end
 end

Seems like there's nothing more to it:

 ./bin/configlet sync --update --exercise yacht
Updating cached 'problem-specifications' data...
Checking exercises...
The `yacht` exercise has up-to-date docs, filepaths, metadata, and tests!
github-actions[bot] commented 5 months ago

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:


Automated comment created by PR Commenter 🤖.

github-actions[bot] commented 5 months ago

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!

angelikatyborska commented 5 months ago

Hi! Thank you for the PR, but I have to reject it. Let me explain.

All practice exercises on Exercism and their test data are defined in a shared central repository called problem specifications. That repository is very well reviewed, each change requires 3 approvals, and generally people taking care of this repository put in a lot of effort figuring out the best test data for any given exercise.

In this repository, I simply copy-and-paste the work done in the central repository with the test data, any only apply changes if there are Elixir-specific reasons to do so. I do not get involved in analyzing the logic behind the exercises because that would be just too much work for me. So I want to keep the test cases in this repo identical with the ones from the problem specifications repo.

To request a change to the test data of the yacht exercise, you would need to edit this file and get a PR approved in the problem specifications repo first. Then I would propagate it here.