New concept exercise: take-a-number-deluxe (genserver)

[angelikatyborska]

Resolves #559
Heavily WIP!

I'm opening this PR to signal that I'm working on this.

Now ready for final review.

Requires #1103 to be merged first to resolve configlet errors and to be able to unlock this exercise.

[angelikatyborska]

The tests, boilerplate, and exemplar solution are ready for review. I want to get those approved before I continue with detailed instructions and hints.

It was really hard to find something that:

• makes sense in the real world,
• uses all three handle_call, handle_cast, and handle_info, and uses handle_call both without user input and with user input.

Overall the exemplar solution is pretty big and this task will require more work than usual, but I don't see a way of making it smaller without compromising on the learning goals.

[angelikatyborska]

Running use GenServer already creates a catch-all handle_info, so I had to get creative to push students to implement it themselves and actually be able to assert they did. At first I wanted just a normal catch-all that calls IO.inspect but I couldn't figure out how to assert on the IO. The normal capture IO in ExUnit doesn't work because the IO happens in a different process.

So I came up with this, and I'm going to justify it with something like: "when testing the initial prototype of the new machine, somebody did something very weird in the client code and ended up sending the old messages meant for the basic Take-A-Number machine to the new GenServer based one instead. They were really annoyed that they didn't receive any responses or any errors, and asked you to add this response for them so they realize what's going on the next time". A bit far-fetched but I was out of ideas.

[jiegillet]

If you want another idea for justifying handle_info, Elixir in Action uses the example of sending messages at regular intervals

def init(_) do
  :timer.send_interval(5000, :cleanup)
  ...
end

You could say that the number queue should be reset once a day.

[Tyyagoo]

Another way to push students to implement the handle_info callback is to use GenServer's own timeout. You can say in history that for "economy" reasons the machine should be turned off after a period of inactivity.

Example:

@timeout 1500

def init(_), do: {:ok, ..., @timeout}

def handle_call(..., ..., ...), do: {:reply, ..., ..., @timeout}
def handle_cast(..., ...), do: {:noreply, ..., @timeout}
def handle_info(:timeout, ...), do: exit(:normal) # energy_is_expensive

That way, if the student doesn't implement this callback, the process will keep running and the tests should fail, since the match all handle_info just calls a :logger.error

Process.sleep(1000)
assert Process.alive?(pid)
Process.sleep(1000)
refute Process.alive?(pid)

This behavior should be easy to test, but these timers in the tests scares me

[angelikatyborska]

@Tyyagoo That sounds brilliant! I totally forgot about the "timeouts" feature. I will try to incorporate your idea.

[jiegillet]

Great work!
I think the exercise makes sense in a "real world" kind of way, and I don't think it will be that hard for students to complete, because all of the heavy lifting is done in TakeANumberDeluxe.State, all they have to do it call the right functions and check for errors.

I would suggest adding some @docs to the TakeANumberDeluxe.State functions, because it's important to quickly grasp what the functions do to pick the right one, but the implementations are not trivial.

[angelikatyborska]

This looks ridiculous, it's not equal to from_list([3]) and the pipe cannot be used 😂

[jiegillet]

That's one reason why we code in Elixir and not in Erlang :)

[github-actions]

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:

• General steps

  • 🏆 Does this PR need to receive a label with a reputation modifier (x:size/{tiny,small,medium,large,massive})? (A medium reputation amount is awarded by default, see docs)

• Any exercise changed

  • 👤 Does the author of the PR need to be added as an author or contributor in <exercise>/.meta/config.json (see docs)?
  • 🔬 Do the analyzer and the analyzer comments exist for this exercise? Do they need to be changed?
  • 📜 Does the design file (<exercise>/.meta/design.md) need to be updated to document new implementation decisions?

• Concept exercise changed

  • 🌲 Do prerequisites and practices in config.json need to be updated?
  • 📖 Does the concept introduction provide all necessary information to solve this exercise?

• Concept exercise tests changed

  • ⚪️ Are all tests un-skipped?
  • 🔢 Are all tests annotated with @tag task_id?
  • 🐈 Can all tests be understood by reading the test's block only (e.g. no module attributes, no setup functions)?

• Concept changed

  • 👤 Does the author of the PR need to be added as an author or contributor in <concept>/.meta/config.json (see docs)?
  • 📖 Does the concept introduction provide all necessary information to solve this exercise?

---

Automated comment created by PR Commenter 🤖.

[angelikatyborska]

I added this as a warning because I'm feeling a bit nervous about how big this exercise ended up being. Do you think this is a good idea?

[jiegillet]

Honestly, I don't think it's that advanced.
I first implemented a GenServer on my own in the bank-account exercise, and it was very very smooth, the official documentation is very clear for anyone who needs more help.

It may have been complicated to implement TakeANumberDeluxe.State but the students don't have to, all they have to do is connect the dots, there is basically no logic to implement.

I think top-secret was more complex.

[angelikatyborska]

Looks like :queue.delete is only available from OTP 24. @jiegillet what would you propose as a fix? Back to my weird list-based queue? Or add a check with function_exported?(:queue, :delete, 2) and use to_list, Enum.filter, and from_list to fake delete?

[jiegillet]

None of these suggestions sparked joy, so I implemented and pushed a queue module. Feel free to revert if it's not to your liking :)
I figured this exercise is already complicated, so this won't make a real difference :)

[angelikatyborska]

Thank you! I'm glad you didn't make me implement a queue myself 😁. I just added an explanation comment at the top of the queue module.

[angelikatyborska]

Something vague is broken with Elixir 1.9 + OTP 20 according to CI:

  1) test start_link/1 min and max numbers get validated (TakeANumberDeluxeTest)
     test/take_a_number_deluxe_test.exs:24
     ** (EXIT from #PID<0.122.0>) :invalid_configuration

It will take me a while to debug because installing OTP below 22 doesn't work (either out of the box or at all?) on my new mac. I will have to debug via Docker.

[angelikatyborska]

I had to build my own Docker image for OTP 20 because the highest Elixir version on DockerHub with OTP 20 is 1.6.

For some reason, start_link sends an exit signal in OTP 20 🤷 that shouldn't happen as far as I understand the docs.

We can either add a hack to the test like this:

otp_version = String.to_integer(System.otp_release())

if otp_version <= 20, do: Process.flag(:trap_exit, true)

# assert stuff

if otp_version <= 20, do: Process.flag(:trap_exit, false)

Or drop OTP 20 support and pair 1.8 and 1.9 with OTP 21. Or try OTP 20.3 instead of 20.0 on CI, hoping that this was a bug that they somehow fixed at some point.

According to https://www.erlang.org/patches/otp-21.0

OTP 21 was first released on 2018-06-19, almost 4 years ago, and OTP 20 was last released on 2020-02-28, 2 years ago.

[jiegillet]

Great work! You had to jump through so many hoops on this one :)

I left a bunch of small comments before we merge.

[angelikatyborska]

@jiegillet Big thanks for the two huge reviews 🙏 I am so happy we will finally fill the GenServer shaped hole in the syllabus...