Mirror of forum.nim-lang.org

4089 :: [best practice] we should use `check` or `require` instead of `echo` + `discard` magic in tests

• timothee (orginal) [2018-07-30T20:07:55+02:00] view original

  I always wondered why unittests used this idiom: discard """ ...""" + echo or write (eg: see [1]).

  Instead the following should be used IMO except in rare cases where echo is needed:

  import std/unittest
  check: # or require if we want to stop running tests on this file on 1st error
    expr1 == expected1
    expr2 == expected2
    ..
    
  Run

  benefits:

  • easier to read/write/edit (the expected value is next to the expression being evaluated)

  Recommendation:

  • keep allowing old pattern but encourage using new pattern
  • encourage using new pattern in new code / tests
  • point users to a reference file treference.nim in contributing.md which has latest best practices for writing a test; it could show using both new pattern and an example of case where discard based pattern is justified.

  [1] eg recent PR: 404f0d64afa5cb18c54c52d261609f60a00cdb46 tests/misc/tsizeof.nim:

  discard """
    file: "tsize.nim"
    output: "40 3 12 32"
  """
  write(stdout, sizeof(TMyRecord))
  echo ' ', mysize1, ' ', mysize2, ' ',mysize3
  Run

---

• GULPF (orginal) [2018-07-30T22:06:11+02:00] view original

  I think most new tests use doAssert instead of echo. But even doAssert leads to a terrible testing experience. If the test is doAssert x == y you have no way of knowing the values of x and y when the test fails. Another issue is that doAssert always exists at the first failure. The unittest module fixes both of these issues, but I think it will be hard to convince Araq that it should be used more :-)

---

• Araq (orginal) [2018-07-30T22:07:22+02:00] view original

  I don't like unittest.nim at all. For all sorts of reasons, historically, unittest would trigger compiler bugs that the test itself wouldn't. unittest's implementation is far too messy and tests should be simple. Tests that are run by Nim's "testament" tool should use doAssert instead.

---

• gemath (orginal) [2018-07-31T09:59:18+02:00] view original

  doAssert doesn't behave as expected in koch tests for me: adding doAssert(false) at the top of tests/stdlib/tparsesql.nim and then running

   ./koch test run stdlib/tparsesql.nim
  Run

  in the top directory of a Nim fork results in

   PASS: tparsesql.nim C
  Run

  This better be some flaw in my fork.

---

• GULPF (orginal) [2018-07-31T10:08:35+02:00] view original

  koch doesn't execute test files by default. To make koch execute a file, either action: run or output: <something> must be included in the test header. The fact that tparsesql doesn't use action: run looks like a bug.

---

• gemath (orginal) [2018-07-31T10:26:56+02:00] view original

  Is the koch command line correct? If it isn't, shouldn't the command exit with an error? If it is, shouldn't the fact that run demands a run test, but action: run is missing from the test source mean that the test fails?

---

• gemath (orginal) [2018-07-31T12:24:43+02:00] view original

  @timothee Your point is valid IMHO, with a caveat:

  • if unittest is as flawed as @Araq says, there may be better ways to achieve some of the things check and require provide: doAssert has a msg parameter to give some info to the user, its default value could be changed to something better than an empty string, which it is now. Also, a nonTerminating macro which wraps every doAssert in a try: block could emulate check in that a failed test doesn't exit.
  • the discard """ ... """ block would still be there, because as @GULPF pointed out, it is also used for local test configuration. Things would be just as non-obvious as they are now: just look at stdlib/tparsesql.nim, it's full of doAssert statements which are silently ignored because someone forgot to add a line to the magical discard string. If we want people to accept a new best practice for testing, it should be one which completely replaces that thing with macros. And when these macros are not used, the test should behave exactly as a person expects who only knows about doAssert.

---

• mratsim (orginal) [2018-07-31T12:37:16+02:00] view original

  Can't we just overhaul unittest?

---

• Araq (orginal) [2018-07-31T13:13:45+02:00] view original

  Unittesting is not good enough for Nim, we also have tests that check the compiler outputs warnings/errors etc. How are you gonna "unittest" that? The Nim compiler uses integretation tests, not unit tests. Not too mention that the tester supports testing the different backends, runs tests in parallel, etc etc. Instead we should make "testament" a well documented tool that every Nim project can use. A library for this is the wrong idea, we need a tool, not a library. You also want process isolation and ideally timeout handling.

---

• andrea (orginal) [2018-07-31T15:17:53+02:00] view original

  I'd say both are needed. Testament certainly works well for the compiler, but a standard library unit testing module is very useful and enough for most libraries

---