common review comments

[timotheecour]

links

• https://nim-lang.github.io/Nim/contributing.html
• recommend this kind of changes to improve tests: https://github.com/nim-lang/Nim/pull/18089

TODO

• [ ] turn this into a proper document or merge it to https://nim-lang.github.io/Nim/contributing.html, but not until we have easy way to write anchors so it's easy to refer to an individual entry
• [ ] we should write an idiomatic test for use as reference for future tests · Discussion #495 · timotheecour/Nim

[timotheecour]

• [ ] standard library header must use <>, not "":

func builtin_rotl8(value: cuchar, shift: cint): cuchar {.importc: "__rolb", header: "x86intrin.h".}
=>
func builtin_rotl8(value: cuchar, shift: cint): cuchar {.importc: "__rolb", header: "<x86intrin.h>".}

[timotheecour]

• [ ] DRY: doc comments should not repeat what's implied by proc declaration example:

  func rotl[T: SomeUnsignedInt](value: T, rot: int32): T {.inline.} =
  ## Left-rotate bits in a `SomeUnsignedInt` value.
  =>
  ## Left-rotate bits in `value`.

[timotheecour]

• [ ] ideally tests should be tested in c, js, vm, using following pattern:

all tests run under a single main template:

discard """
  targets: "c js"
"""

template main =
  block: # foo
     doAssert foo(1) == 1
     doAssert foo(1, 2) == 3

  block: # bar
     doAssert bar()
     doAssert not compiles(bar("abc"))

  block: # bug #1234
    ...

static: main()
main()

[timotheecour]

• [ ] use block: # foo instead of block foo:; in particular when testing for a symbol foo

block foo: should only be used if the code refers to foo as a block label (eg via break foo); nim might in future issue warnings for labels without code referring to it

[timotheecour]

inline definitions of variables used only once, eg:

    let
      a = some(42)
      b = none(int)
      c = some(-11)
    assert a.flatMap(doublePositives) == some(84)
    assert b.flatMap(doublePositives) == none(int)
    assert c.flatMap(doublePositives) == none(int)

=>

    assert some(42).flatMap(doublePositives) == some(84)
    assert none(int).flatMap(doublePositives) == none(int)
    assert some(-11).flatMap(doublePositives) == none(int)

it results in more readable tests where all the context is in 1 line

[timotheecour]

function tested goes on LHS for clarity:

    assert {true, false} == fullSet(bool)
=>
    assert fullSet(bool) == {true, false}

[timotheecour]

• [ ] error messages should not be redundant:

  # bad:
  assert bounds.a <= bounds.b, " bounds b should be larger or equal to bounds.b"
  =>
  # good:
  assert bounds.a <= bounds.b

the 1st form shows:

/t11954.nim(13, 10) `bounds.a <= bounds.b`  bounds b should be larger or equal to bounds.b [AssertionDefect]

the 2nd shows:

t11954.nim(13, 10) `bounds.a <= bounds.b`  [AssertionDefect]

which has exactly the same information. Simpler is better, both for reading and writing code and for developpers who will encounter this error msg.

note: runtime context

Currently assert + friends (unlike unittest.check) doesn't show runtime values on an assertion failure, but in future this could be implemented at least for the most common cases, controlled by a flag, maybe --errorcontexts:on.

Until then, to show runtime values on errors, we need:

assert bounds.a <= bounds.b, $(bounds.a, bounds.b)

this shows:

t11954.nim(13, 10) `bounds.a <= bounds.b` (4, 3) [AssertionDefect]

which is IMO perfectly readable, especially if this becomes a pattern; simplicity wins here. We could also introduce a reusable helper macro to format those, eg:

assert bounds.a <= bounds.b, prettyMsg(bounds.a, bounds.b)

which would then show:

t11954.nim(13, 10) `bounds.a <= bounds.b` (bounds.a: 4, bounds.b: 3) [AssertionDefect]

[timotheecour]

• [ ] don't hardcode ports in tests (eg: Port(8800), etc), it leads to flaky tests; instead use Port(0) + getPort

refs https://github.com/nim-lang/Nim/pull/14327, https://github.com/nim-lang/Nim/pull/17559

[timotheecour]

• [ ] by default, PRs need to target branch devel, not latest stable branch. if a PR targets the wrong branch, instead of closing and re-opening a new PR, the following can work:

either read github docs on how to do this, or do the following to avoid lots of conflicts:

before changing back target branch to devel, do this:

• update your local devel branch with git pull --ff-only
• push it
• rebase your PR branch against devel
• only then, change back the PR's target branch back to devel (via github UI)

[timotheecour]

• [ ] write test artifacts in either a gitignored directory (import stdtest/specialpaths; const dir = buildDir) or in getTempDir()

rationale:

• avoid creating tests files scattered among sources; even if the test cleans up those, the test may crash or the testament may be interrupted before cleanup happens
• avoid interference with other tests that could generate same file (tests are run in parallel, making this worse)
• stdtest/specialpaths.buidDir is already gitignored