testament: support per-target knownIssues (#834)

## Summary

Add support for per-target `knownIssue` specification to testament.

Previously, only the full test (including all targets) could be marked
as a "known issue". To emulate per-target `knownIssue`s, tests had to
explicitly disable the target for which the test was failing and add a
separate comment, but this has the issue of `--tryfailing` not applying.

The `knownIssue` key in the test specifications now supports a
*sub-key*, like so: `knownIssue.target`, where `target` is one of `c`,
`js`, or `vm`. A `knownIssue` key without a sub-key means "applies to
all targets", and specifying multiple targets works by separating them
via spaces.

## Details

At the core of the changes is that `knownIssues` are now per-target
instead of per-spec. The spec parser already supports dots in keys, so
the only addition there is splitting the parsed key string into the main
and sub part prior to processing the key.

Since "has known issue" is now a per-run attribute instead of a per-spec
one, disabling a test early by returning `reKnownIssue` from
`computeEarly` cannot work anymore -- everything related to whole-spec
known issues is removed. Instead, test-run descriptions are now always
generated, but then later skipped by `run` (which now accepts a
`runKnownIssues` parameter) if they have known issues and trying those
is disabled.

Since skipped "known issue" test runs would have no associated "known
issue" information in the backend otherwise, `addResult` for `TestRun`
now always includes the runs "known issues" information.

Tests using the disable + comment approach are adjusted to use per-
target `knownIssue`s, but without checking whether the issue is still
current.

---------

Co-authored-by: Saem Ghani <saemghani+github@gmail.com>

doc/testament.rst

@@ -127,7 +127,16 @@ Test execution options
 
 - ``knownIssue`` description of the test that currently fails but should
   execute successfully; it is a known bug that must be fixed in the future.
-  Can be used several times in specification.
+  Can be used several times in specification. One can also specify the
+  affected targets.
+
+  .. code-block:: nim
+
+    knownIssue.vm: "..."   # the test only fails with the vm target
+    knownIssue.c js: "..." # fails with both the c and js target
+
+  A standalone ``knownIssue`` key means that all selected targets are
+  affected by the issue(s).
 
 Compiler output assertions
 --------------------------

testament/categories.nim

@@ -319,7 +319,7 @@ proc isJoinableSpec(spec: TSpec, targets: set[TTarget], early: TResultEnum): boo
     not fileExists(spec.file.changeFileExt("nim.cfg")) and
     not fileExists(parentDir(spec.file) / "nim.cfg") and
     spec.cmd.len == 0 and
-    early notin {reDisabled, reKnownIssue} and
+    early notin {reDisabled} and
     not spec.unjoinable and
     spec.exitCode == 0 and
     spec.input.len == 0 and
@@ -331,6 +331,13 @@ proc isJoinableSpec(spec: TSpec, targets: set[TTarget], early: TResultEnum): boo
     spec.outputCheck != ocSubstr and
     spec.ccodeCheck.len == 0 and
     (spec.targets * targets != {} or spec.targets == {})
+  if result:
+    # the test must have no known issues
+    for it in spec.knownIssues.items:
+      if it.len > 0:
+        result = false
+        break
+
   if result:
     if spec.file.readFile.contains "when isMainModule":
       result = false
@@ -555,17 +562,14 @@ proc processCategory(r: var TResults, cat: Category, targets: set[TTarget],
     for i, name in files:
       let test = makeTest(name, options, cat)
       var res = computeEarly(test.spec, test.inCurrentBatch)
-      if res == reKnownIssue and runKnownIssues:
-        # we want to still run the test
-        res = reSuccess
-      elif runJoinableTests or
+      if runJoinableTests or
           not isJoinableSpec(test.spec, targets, res) or
           cat.string in specialCategories:
         discard "run the test"
       else:
         res = reJoined
       produceRuns r, test, res, runs
-      run(r, runs)
+      run(r, runs, runKnownIssues)
       runs.setLen(0) # prepare for the next test
       inc testsRun
     if testsRun == 0:

testament/specs.nim

@@ -121,7 +121,7 @@ type
     timeout*: float ## in seconds, fractions possible, but don't rely on
     ## much precision
     inlineErrors*: seq[InlineError] ## line information to error message
-    knownIssues*: seq[string] ## known issues to be fixed
+    knownIssues*: array[TTarget, seq[string]] ## known issues to be fixed
     labels*: seq[string] ## user-added metadata
 
 proc getCmd*(s: TSpec): string =
@@ -289,6 +289,16 @@ proc parseSpecifiedTargets*(value: string): set[SpecifiedTarget] =
       else: raise newException(ValueError,
                                "invalid target specificatoin: '$#'" % v)
 
+proc splitKey*(key: string): tuple[key, sub: string] =
+  ## Splits the key (after normalizing) into the main and sub-key part.
+  let
+    key = normalize(key)
+    p = find(key, '.')
+  if p == -1:
+    result = (key, "")
+  else:
+    result = (key[0..<p], key[p+1..<key.len])
+
 proc addLine*(self: var string; a: string) =
   self.add a
   self.add "\n"
@@ -317,7 +327,7 @@ proc parseSpec*(filename: string,
     var e = next(p)
     case e.kind
     of cfgKeyValuePair:
-      let key = e.key.normalize
+      let (key, subKey) = splitKey(e.key)
       const allowMultipleOccurences = ["disabled", "ccodecheck" , "knownissue"]
         ## list of flags that are correctly handled when passed multiple times
         ## (instead of being overwritten)
@@ -491,7 +501,13 @@ proc parseSpec*(filename: string,
         case e.value.normalize
         of "n", "no", "false", "0": discard
         else:
-            result.knownIssues.add e.value
+          # no sub-key means "applies to all targets"
+          let targets =
+            if subKey.len == 0: {low(TTarget)..high(TTarget)}
+            else:               parseTargets(subKey)
+
+          for t in targets.items:
+            result.knownIssues[t].add e.value
       of "labels":
          discard """
          Adding only key support for now.

testament/testament.nim

@@ -592,7 +592,7 @@ proc addResult(
       debugInfo: run.debugInfo,
       outCompare: outCompare,
       success: success,
-      knownIssues: if isNil(output): @[] else: run.test.spec.knownIssues,
+      knownIssues: run.test.spec.knownIssues[run.target],
       inCurrentBatch: run.test.inCurrentBatch,
       expected: expected,
       given: given
@@ -603,15 +603,14 @@ proc addResult(
 proc addResult(r: var TResults, test: TTest, reason: TResultEnum) =
   ## Report test failure/skip etc to backend, end user (write to command-line)
   ## and so on.
-  const allowedTestStatus = {reInvalidSpec, reDisabled, reKnownIssue, reJoined}
+  const allowedTestStatus = {reInvalidSpec, reDisabled, reJoined}
   doAssert reason in allowedTestStatus,
            "Test: $1 with status: $2, should have ran" %
               [test.getName(), $reason]
   let
     given =
       case reason
       of reInvalidSpec: test.spec.parseErrors
-      of reKnownIssue: test.spec.knownIssues.join("\n")
       else: ""
     param = ReportParams(
       duration: epochTime() - test.startTime,
@@ -623,7 +622,7 @@ proc addResult(r: var TResults, test: TTest, reason: TResultEnum) =
       debugInfo: "",
       success: reason,
       inCurrentBatch: test.inCurrentBatch,
-      knownIssues: test.spec.knownIssues,
+      knownIssues: @[],
       expected: "",
       given: given,
       outCompare: nil
@@ -1043,7 +1042,7 @@ proc testSpecHelper(r: var TResults, run: var TestRun) =
     if timeout > 0.0 and duration > timeout:
       res.success = reTimeout
 
-  if run.expected.knownIssues.len > 0:
+  if run.expected.knownIssues[run.target].len > 0:
     # the test has known issue(s) and is expected to fail
     if res.success == reSuccess:
       # it didn't fail
@@ -1063,22 +1062,24 @@ proc testSpecHelper(r: var TResults, run: var TestRun) =
   r.addResult(run, res.expected, res.given, res.success,
               addr given, res.compare)
 
-proc targetHelper(r: var TResults, run: var TestRun) =
-  inc(r.total)
-  if run.target notin gTargets:
-    r.addResult(run, "", "", reDisabled)
-    inc(r.skipped)
-  elif simulate:
-    inc count
-    msg Undefined: "testSpec count: " & $count & " expected: " & $run.expected
-  else:
-    testSpecHelper(r, run)
-
-proc run(r: var TResults, runs: var openArray[TestRun]) =
+proc run(r: var TResults, runs: var openArray[TestRun], runKnownIssues: bool) =
   ## Executes the given `runs`.
   for run in runs.mitems:
     run.startTime = epochTime()
-    targetHelper(r, run)
+    inc(r.total)
+    # XXX: remove the usage of globals here
+    if run.target notin gTargets:
+      r.addResult(run, "", "", reDisabled)
+      inc(r.skipped)
+    elif run.expected.knownIssues[run.target].len > 0 and not runKnownIssues:
+      # tests with known issues are not tried
+      r.addResult(run, "", "", reKnownIssue)
+      inc(r.skipped)
+    elif simulate:
+      inc count
+      msg Undefined: "testSpec count: " & $count & " expected: " & $run.expected
+    else:
+      testSpecHelper(r, run)
 
 func nativeTarget(): TTarget {.inline.} =
   targetC
@@ -1118,8 +1119,6 @@ proc computeEarly(spec: TSpec, inCurrentBatch: bool): TResultEnum =
     reDisabled # manually skipped
   elif not inCurrentBatch:
     reDisabled
-  elif spec.knownIssues.len > 0:
-    reKnownIssue
   else:
     reSuccess
 
@@ -1168,7 +1167,7 @@ proc testSpec(r: var TResults, test: TTest) =
   let res = computeEarly(test.spec, test.inCurrentBatch)
   var runs: seq[TestRun]
   produceRuns r, test, res, runs
-  run(r, runs)
+  run(r, runs, false)
 
 proc testSpecWithNimcache(
     r: var TResults, test: TTest; nimcache: string) {.used.} =

tests/lang_callable/closure/tinfer_closure_for_nestedproc.nim

@@ -1,11 +1,11 @@
 discard """
   action: compile
-  target: "!vm"
+  knownIssue.vm: '''
+    The `strtabs` module imports the ``std/os`` module, which isn't
+    yet supported for the VM target
+  '''
 """
 
-# knownIssue: the `strtabs` module imports the ``std/os`` module, which isn't
-#             yet supported for the VM target
-
 # bug #9441
 import std/strtabs
 

tests/lang_callable/generics/tobjecttyperel.nim

@@ -1,5 +1,8 @@
 discard """
-  target: "!vm"
+  knownIssue.vm: '''
+    Fails for the VM target because ``cgirgen`` emits the incorrect conversion
+    operator for ``ref`` types involving generics.
+  '''
   output: '''(peel: 0, color: 15)
 (color: 15)
 17
@@ -9,10 +12,6 @@ cool
 test'''
 """
 
-# knownIssue: fails for the VM target because ``cgirgen`` emits the
-#             incorrect conversion operator for ``ref`` types involving
-#             generics
-
 # bug #5241
 type
   BaseFruit[T] = object of RootObj

tests/lang_callable/generics/treentranttypes.nim

@@ -1,5 +1,5 @@
 discard """
-target: "!vm"
+knownIssue.vm: '''"same" output but float formatting is off'''
 output: '''
 (10, ("test", 1.2))
 3x3 Matrix [[0.0, 2.0, 3.0], [2.0, 0.0, 5.0], [2.0, 0.0, 5.0]]
@@ -17,8 +17,6 @@ output: '''
 '''
 """
 
-# disabled on VM: "same" output but float formatting is off (knownIssue)
-
 # https://github.com/nim-lang/Nim/issues/5962
 
 type

tests/lang_callable/method/tmultim.nim

@@ -1,5 +1,8 @@
 discard """
-  target: "!vm"
+  knownIssue.vm: '''
+    The ``of`` operator only considers the static type when using the
+    VM backend
+  '''
   joinable: false
   output: '''
 collide: unit, thing
@@ -13,9 +16,6 @@ do nothing
 '''
 """
 
-# knownIssue: the ``of`` operator only considers the static type when using
-#             the VM backend
-
 # tmultim2
 type
   TThing = object {.inheritable.}

tests/lang_callable/template/template_issues.nim

@@ -1,5 +1,5 @@
 discard """
-target: "!vm"
+knownIssue.vm: true
 output: '''
 0
 a
@@ -12,8 +12,6 @@ true
 '''
 """
 
-# VM disabled: needs a deeper dive (knownIssue)
-
 import std/[macros, json]
 
 

tests/lang_callable/template/template_various.nim

@@ -1,5 +1,5 @@
 discard """
-target: "!vm"
+knownIssue.vm: "std/streams and/or std/os don't work for the VM"
 output: '''
 i2416
 33
@@ -35,8 +35,6 @@ bar7
 '''
 """
 
-# disabled on VM: std/streams and/or std/os don't work for the VM (knownIssue)
-
 import std/macros
 
 

tests/lang_experimental/views/tprocedural_views.nim

@@ -1,14 +1,15 @@
 discard """
-  targets: "c js !vm"
+  targets: "c js vm"
   description: '''
     Tests for indirect calls where the callee is an expression evaluating to a
     view
   '''
+  knownIssue.vm: '''
+    Semantic analysis produces incorrect AST for tuple initialization, causing
+    VM access violation errors at run-time
+  '''
 """
 
-# knownIssue: semantic analysis produces incorrect AST for tuple initialization,
-#             causing VM access violation errors at run-time
-
 {.experimental: "views".}
 
 type Proc = proc(x: int): int {.nimcall.}

tests/lang_experimental/views/tviews_in_object.nim

@@ -1,14 +1,13 @@
 discard """
-  targets: "c !js vm"
+  targets: "c js vm"
   matrix: "--experimental:views"
   description: '''
     Tests for direct single-location views used as the type of
     object fields. Only read access is tested here.
   '''
+  knownIssue.js: "multiple problems with the JavaScript code generator"
 """
 
-# knownIssue: multiple problems with the JavaScript code generator
-
 # note: all the tests here are run in the context of procedures; tests for
 # views in the context of top-level code should go elsewhere
 

tests/lang_objects/destructor/tlvalue_conversions.nim

@@ -3,12 +3,11 @@ discard """
     Tests for assignments of types with lifetime hooks where l-value
     conversions are involved
   '''
-  targets: "c !js !vm"
+  targets: "c js vm"
   matrix: "--gc:arc --cursorInference:off"
+  knownIssue.js vm: "`--gc:arc` is not supported"
 """
 
-# knownIssues: both the JS and VM target don't yet support ``--gc:arc``:option:
-
 import mhelper
 
 block sink_from_ref_with_conversion:

tests/lang_stmts/casestmt/tcase_with_uint_values.nim

@@ -1,14 +1,15 @@
 discard """
-  targets: "c !js vm"
+  targets: "c js vm"
   description: '''
     Ensures that case statements work with uint operands and of-branch values
     not representable with the same-sized signed integer type
   '''
+  knownIssue.js: '''
+    Compiles correctly for JS, but doesn't work at run-time because
+    of the improper large integer support
+  '''
 """
 
-# knownIssue: compiles correctly for JS, but doesn't work at run-time because
-#             of the improper large integer support
-
 proc test[T: uint64|uint; S]() =
   const Border = T(high(S)) # the highest possible signed integer value