Add doc tests to the doc concept and corresponding city-office exercise

[maco]

The docs concept didn't explain doc tests or check that people could write them, and that seemed like a gap. This fills that gap.

(The test for this is based on ExUnit's own tests.)

[github-actions[bot]]

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]

(Note: I'm leaving a short, not well-research opinion first, to kick off a discussion with other maintainers)

Hmm, I'm not entirely sure how to feel about this idea. On one hand, doctests are awesome and I would expect all Elixir devs to know them. On the other, learning writing tests was never in scope for students on Exercism because tests are always already given.

We also need to check if this is going to work in the test runner. If it works in the test runner, and if we agree that it's a good idea to teach doctests, then I think this is a good exercise to add them to. I would probably even want to add a doctest writing step to each function, each as a separate step in instructions, so that the exercise is symmetrical.

@jiegillet @neenjaw I'm looking forward to your opinions here

[angelikatyborska]

I did a quick check in the test runner (commit https://github.com/exercism/elixir-test-runner/commit/b1ade5c98160364163e63e2d6eb7c2b21502db62 based on Jie's unmerged work in https://github.com/exercism/elixir-test-runner/pull/86, command: ./bin/smoke_test.sh). It doesn't work in the test runner. The new test fails. I'm not yet sure why.

parsing /tmp/solution_pKj7YEPWYN/test/form_test.exs for metadata
transforming /tmp/solution_pKj7YEPWYN/test/form_test.exs
3c3
<   "status": "pass",
---
>   "status": "fail",
118c118,119
<       "message": null,
---
>       "message": "  1) test check_length/2 has 2 passing doctests (FormTest)\n     \u001b[1m\u001b[30mtest/form_test.exs:139\u001b[0m\n     \u001b[31mAssertion with =~ failed\u001b[0m\n     \u001b[36mcode:  \u001b[0massert output =~ \"2 doctests, 0 failures\"\n     \u001b[36mleft:  \u001b[0m\"\u001b[48;5;88m\u001b[0m\"\n     \u001b[36mright: \u001b[0m\"\u001b[32m2 doctests, 0 failures\u001b[0m\"\n     \u001b[36mstacktrace:\u001b[0m\n       test/form_test.exs:146: (test)\n",
>       "name": "test check_length/2 has 2 passing doctests",
120c121
<       "status": "pass",
---
>       "status": "fail",
🔥 test/city-office: expected /tmp/output_lLFCXZHBPy/results.json to equal test/city-office/expected_results.json on successful run 🔥

For readability, formatted message is this:

[jiegillet]

I do like the idea of talking about doctests, and actually, why not adding a testing concept? It's an important topic, and it would be fun to figure out how to test tests :)

I thought that the test-runner could handle doctests, I'll take a closer look soon and figure out why it doesn't here.

[jiegillet]

~~I played around for a bit, and I think the test-runner is not happy because before running the tests, it's modifying the source code of the tests by traversing the AST, getting rid of the docs and therefore the doctests in the process.~~

~~It's the second use case in as many weeks where we would actually prefer keeping the comments. Since 1.13 it should be possible to keep the comments, but it's a bunch of work. What do you think @angelikatyborska?~~

EDIT: wait, that's wrong, only the test files are modified, not the submitted solution... Sorry, wrong guess!

I also noticed that the CI is failing:

* test check_length/2 has 2 passing doctests (60023.9ms)
  1) test check_length/2 has 2 passing doctests (FormTest)
     test/form_test.exs:207
     ** (exit) exited in: GenServer.call(ExUnit.Server, {:take_async_modules, 1}, 60000)
         ** (EXIT) time out
     stacktrace:
       (elixir) lib/gen_server.ex:989: GenServer.call/3

I haven't gotten to the bottom of that yet.

[angelikatyborska]

Great! If we can make the tests pass without modifying the test runner like Jie says, and is Jie likes the idea of the doc tests too, then I think we should add them 🎉.

I think ideally every exercise step that says "Add documentation and a typespec to the ... function." would also be extended to include adding a doctest. It's great that the doctest macro accepts an :only option, that will allow us to create a separate unit test for each step.

@maco are you interested in making those changes and do you have time for that? If not, I can also take over.

[maco]

Yes, I'm fine to make those changes.

[maco]

Ok, I made a bunch of changes, but not the part involving sed yet. I need to noodle that a bit because crossing newline boundaries with regex is tricky.

[jiegillet]

@maco do you still have time/motivation to work on this, or would you like us to take over?

[maco]

@jiegillet a month later, with fresh eyes, I realize the DefModule regex starts at ^ to avoid matching nested ones, and I can do the same on the use ExUnit.Case regex.

I'm unclear on why this timed out in CI, though.

[jiegillet]

I took a look at the CI failure, but I'm not sure what to do. The problem is probably related to running tests within tests (GenServers within GenServers...). It fails in Elixir 1.8 and 1.9, and succeeds in 1.12 and 1.13 (1.10 has been hanging for 17 minutes, not a good sign).

Not the cleanest suggestion, but we could simply rely on checking that well formatted doctests have been written. The doctests are being tested, so if they fail, it will be caught.

[neenjaw]

If the code is correct by current elixir syntax/patterns, the easiest thing to do is drop 1.8, 1.9, 1.10 in CI. (Edit: and bump the minimum version supported)

I have no comment on the correctness of this PR, haven't looked at it in detail, but arguably if it doesn't work for those versions, yet does for later ones -- there was a bug/limitation in ex_unit which then was patched/extended.

[angelikatyborska]

Just FYI, I think it's this bug: https://github.com/elixir-lang/elixir/issues/9601

Fixed in https://github.com/elixir-lang/elixir/commit/bae1cc7cdf1a5beb27530ff8d85869e4c845cda6 which was part of 1.10, but not lower

[angelikatyborska]

It fails in Elixir 1.8 and 1.9, and succeeds in 1.12 and 1.13 (1.10 has been hanging for 17 minutes, not a good sign).

I tested in the official Docker containers elixir:1.10-alpine and elixir:1.11-alpine, and it hangs for a very long time in those too, which means only 1.12 and 1.13 work as expected with this approach.

[jiegillet]

I don't think we want to drop all these versions :(

Also, after all this trouble, I checked and elixir-test-runner doesn't actually run doctests at the moment. When adding the line doctest Form to city-office/test/form_test.exs, the doctests do run, and test runner output is

{
  "message": null,
  "status": "pass",
  "tests": [
    {
      "message": null,
      "metadata-error": "metadata missing for test",
      "name": "doctest Form.blanks/1 (1)",
      "output": null,
      "status": "pass",
      "test_code": null
    },
   ...

when everything passes and

{
  "message": null,
  "status": "fail",
  "tests": [
    {
      "message": "  1) doctest Form.blanks/1 (1) (FormTest)\n     \u001b[1m\u001b[30mtest/form_test.exs:3\u001b[0m\n     \u001b[31mDoctest failed\u001b[0m\n     \u001b[36mdoctest:\u001b[0m\n       iex> Form.blanks(3)\n       \"XXXXX\"\n     \u001b[36mcode:  \u001b[0mForm.blanks(3) === \"XXXXX\"\n     \u001b[36mleft:  \u001b[0m\"XXX\"\n     \u001b[36mright: \u001b[0m\"XXX\u001b[32mXX\u001b[0m\"\n     \u001b[36mstacktrace:\u001b[0m\n       lib/form.ex:17: Form (module)\n",
      "metadata-error": "metadata missing for test",
      "name": "doctest Form.blanks/1 (1)",
      "output": null,
      "status": "fail",
      "test_code": null
    },
...

when the first doctest fails (and everything else passes). The tests do run and the final result is accurate, but the messages are really bad. It would require quite a bit of work to make it nicer.

I have a terrible, terrible, hacky alternative, please let me know what you think. The following test works

    test "has a passing doctest, regex edition" do
      doctest_regex = ~r'''
          iex> Form.blanks\((?<input>\d*)\)
          "(?<output>.*)"
      '''

      assert_doc({:blanks, 1}, doctest_regex)

      assert %{"input" => input, "output" => output} =
               doc_named_captures({:blanks, 1}, doctest_regex)

      assert Form.blanks(String.to_integer(input)) == output
    end

where doc_named_captures is the exact copy of assert_doc expect for the last line that is changed to Regex.named_captures(unquote(expected_doc), actual_doc) (to be be refactored later I guess).

[jiegillet]

@maco an Exercism-wide cleanup wave is on the rise. Are you still interested in pursuing this PR, maybe along the line of what I suggested above? If not, I think we will drop it.