improve error for recursion limit

[ee7]

Still WIP. The tests probably fail. And I'll have to read the spec again to find out if this is supposed to be a top-level error or not.

~I'll probably do "if the error message mentions the call depth limit, replace the error message with something else"~ Nah

Edit: seems like it should be a top-level fail.

[ee7]

@ErikSchierboom given a test file

import std/unittest
import identity

suite "Identity Function":
  test "identity function of 1":
    check identity(1) == 1

  test "identity function of 2":
    check identity(2) == 2

  test "identity function of 3":
    check identity(3) == 3

and this solution file, which hits the recursion limit in the second test

func identity*(n: int): int =
  if n == 2:
    identity(n)
  else:
    n

is the expected results.json the below?

{
  "version": 2,
  "status": "fail",
  "tests": [
    {
      "name": "identity function of 1",
      "status": "pass",
      "output": ""
    },
    {
      "name": "identity function of 2",
      "status": "error",
      "message": "Traceback (most recent call last)\n/nim/lib/pure/unittest.nim(654) test_identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\n(1874 calls omitted) ...\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim identity\nError: call depth limit reached in a debug build (2000 function calls). You can change it with -d:nimCallDepthLimit=<int> but really try to avoid deep recursions instead.\n",
      "output": ""
    }
  ]
}

And if not, could you write what it should be?

Please ignore the lack of the test_code property.

The docs say this about the top-level status

The following overall statuses are valid:

pass: All tests passed fail: At least one test failed error: To be used when the tests did not run correctly (e.g. a compile error, a syntax error)

and this about the top-level message:

Where the status is error (the tests fail to execute cleanly), the top level message key should be provided. It should provide the occurring error to the user. As it is the only piece of information a user will receive on how to debug their issue, it must be as clear as possible. For example, in Ruby, in the case of a syntax error, we provide the error and stack trace. In compiled languages, the compilation error should be provided. The top level message value is not limited in length.

and this about the test-level status:

The following per-test statuses are valid:

pass: The test passed fail: The test failed error: The test errored

I'm confused because I think the message docs imply that, for compiled languages, the top-level error is only for compilation errors. But this is a run-time error where the first test passes and the second test errors.

The nim test runner currently produces this for a run-time exception, which the runner currently handles better than something like hitting the recursion limit: https://github.com/exercism/nim-test-runner/blob/4772480c87822b3f2f512b2b81965d88f99ec669/tests/error/runtime_exception_in_solution/expected_results.json#L1-L12

[ErikSchierboom]

I'm confused because I think the message docs imply that, for compiled languages, the top-level error is only for compilation errors. But this is a run-time error where the first test passes and the second test errors.

The top-level status should only be error when the tests couldn't even be executed. Usually this means there is a syntax error somewhere in the code. If any of the tests were executed, the status property should be null or omitted.

In your case, I'd expect:

{ 
   "version": 2, 
   "status": "fail", 
   "tests": [ 
      {
      "name": "identity function of 1",
      "status": "pass",
      "output": ""
    },
    {
      "name": "identity function of 2",
      "status": "error",
      "message": "Traceback (most recent call last)\n/nim/lib/pure/unittest.nim(654) test_identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\n(1874 calls omitted) ...\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim(3) identity\nidentity.nim identity\nError: call depth limit reached in a debug build (2000 function calls). You can change it with -d:nimCallDepthLimit=<int> but really try to avoid deep recursions instead.\n",
      "output": ""
    } 
   ] 
 } 

So the recursion test case is marked with error as you could argue that an error occurs in the test case, but you could also argue it is "just" a failure to pass the test. In this case the key thing is that no result is returned, so you can't know if it would be a pass or fail, so error is appropriate here.

BTW error or fail for a test case doesn't matter much in the UI. Only one word is changed in the UI (“Failed” vs “Errored”)

[ee7]

(I edited your post - let me know if it's incorrect).

Thanks, that helps. I've created https://github.com/exercism/docs/pull/175 to try to clarify.

BTW error or fail for a test case doesn't matter much in the UI. Only one word is changed in the UI (“Failed” vs “Errored”)

Ah, I see.

---

Just to confirm: for the case you addressed, the tests array in the results.json should not contain the third test case, correct? That is: for a language that executes the tests in order, one at a time, the tests array should not contain an item after an item with a status of error?

[ErikSchierboom]

Just to confirm: for the case you addressed, the tests array in the results.json should not contain the third test case, correct? That is: for a language that executes the tests in order, one at a time, the tests array should not contain an item after an item with a status of error?

Correct. It does indeed depend on the test runner, as for example the C# test will always run all the tests anyway. But if the tests are executed in order, it's perfectly fine to stop at the error.

[ynfle]

@ee7 Any updates? What still needs to be done?

[ee7]

@ee7 Any updates? What still needs to be done?

I think there was still something I wanted to do, but I'll get back to you.