unittest2.nim: ensure the testTeardownIMPL is performed at the end

[Ivansete-status]

This is important to prevent "failing tests" from failing in an uncontrolled way and generating a SIGSEGV (segmentation fault)

Particularly, when the test body raises an exception, e.g. "assert false", what happened was that the "testTeardownIMPL" was invoked before the exception handling, and that caused errors like:

Traceback (most recent call last, using override)
/home/ivansete/workspace/status/nwaku/vendor/nim-unittest2/unittest2.nim(1154) unittest2
/home/ivansete/workspace/status/nwaku/vendor/nim-unittest2/unittest2.nim(1086) runDirect
/home/ivansete/workspace/status/nwaku/vendor/nim-unittest2/unittest2.nim(1111) runTestX60gensym398
/home/ivansete/workspace/status/nwaku/vendor/nimbus-build-system/vendor/Nim/lib/system/excpt.nim(631) signalHandler
SIGSEGV: Illegal storage access. (Attempt to read from nil?) Segmentation fault (core dumped)

[AlejandroCabeza]

Not going to approve as I don't know the extent of implications this might have in the codebase, but that's one headache less!
Thanks!

[AlejandroCabeza]

Using testTeardownIMPL (in the log message) feels slightly odd to me, but I might be overanalysing things. Everyone who'll read this will (likely) be an engineer, so I don't see any danger 😆

[arnetheduck]

indeed, this should be teardown, which is what the library user sees

[jangko]

Add test please.

[Ivansete-status]

Add test please.

Very good morning @jangko . We've encountered the segmentation fault issue many times.

Particularly, we are now facing it in the following PR: waku-org/nwaku#2269

And the following comment indicates the precise test that is causing it: waku-org/nwaku#2269 (comment)

Let me know if any info or detail is missing.
Cheers

[Ivansete-status]

Thanks @lchenut for this recommendation! I think now we will be more protected 🥳

[arnetheduck]

this except line is redundant (Exception is the superclass of all exceptions) - also, we typically don't use getCurrentException* as they have semantic problems - it's better to name the exception in the except line

[Ivansete-status]

Thank you for the comments @AlejandroCabeza @arnetheduck ! I think now it is better.

@jangko - I'm afraid I cannot implement a proper test that validates the desired behavior. Let me elaborate on it a bit more:
With that PR, I want to make sure that, in case of an exception happening in the test body, the teardown is always called at the very end of the test. Therefore, this is the order that I want to ensure:

1. setup
2. the test body throws an exception, i.e. assert false
3. the exception is properly captured and the test is marked with fail().
4. the teardown is run.

As you can see, I would need to explicitly throw an exception and make the test fail in order to force the desired flow. Something that seems incompatible with a successful test.

On the other hand, the following is the undesired flow:

1. setup
2. the test body throws an exception, i.e. assert false
3. the teardown is run.
4. the exception is captured.

We had a very tricky issue from nwaku point of view, where the teardown is an async proc, which tries to stop a nim-libp2p Switch object. With that previous undesired flow, we got a segmentation fault error and the test was not properly marked as failed.

I hope that makes it clearer.
Cheers

[jangko]

can't we misuse testStatusIMPL?, looks like it is visible in teardown body.

[Ivansete-status]

can't we misuse testStatusIMPL?, looks like it is visible in teardown body.

Apologies but I can't quite understand the question.

The testStatusIMPL() represents indeed the body defined when the teardown template is used.

ℹ️ teardown template definition:

    template teardown(teardownBody: untyped) {.dirty, used.} =
      var testTeardownIMPLFlag {.used.} = true
      template testTeardownIMPL: untyped {.dirty.} = teardownBody

On the other hand, from nwaku, we use the teardown template either directly or through the following template, defined within a nwaku module:

template asyncTeardown*(body: untyped): untyped =
  teardown:
    waitFor((
      proc() {.async.} =
        body
    )())

[Ivansete-status]

I will merge that PR.
@jangko @arnetheduck - ping me if you have a strong opinion against this merge and I will revert it immediately. Apologies for the possible inconvenience.

[jangko]

What I mean is we can use the testStatusIMPL variable to set the test failed or not. But because we are going to force a failed test become an ok test, that's why I called it misusing testStatusIMPL.

suite "a test suite":
  setup:
    debugEcho "SETUP" 
    
  test "something":
    debugEcho "TEST"
    doAssert(false)

  teardown:
    debugEcho "TEARDOWN"
    check testStatusIMPL == TestStatus.FAILED
    testStatusIMPL = TestStatus.OK