testament/azure: major rewrite (#13246)

This commit features a major rewrite of Azure Pipelines integration,
turning the spaghetti it originally was into something maintainable.

Key changes:
- No longer requires a ton of hooks into testament.
- Results are now cached then bulk-uploaded to prevent throttling from
  Azure Pipelines, avoiding costly timeouts.
- A low timeout is also employed to avoid inflated test time.
- The integration is now documented.

testament/azure.nim

@@ -6,90 +6,142 @@
 #    Look at license.txt for more info.
 #    All rights reserved.
 
-import base64, json, httpclient, os, strutils
+import base64, json, httpclient, os, strutils, uri
 import specs
 
 const
-  ApiRuns = "/_apis/test/runs"
-  ApiVersion = "?api-version=5.0"
-  ApiResults = ApiRuns & "/$1/results"
-
-var runId* = -1
+  RunIdEnv = "TESTAMENT_AZURE_RUN_ID"
+  CacheSize = 8 # How many results should be cached before uploading to
+                # Azure Pipelines. This prevents throttling that might arise.
 
 proc getAzureEnv(env: string): string =
   # Conversion rule at:
   # https://docs.microsoft.com/en-us/azure/devops/pipelines/process/variables#set-variables-in-pipeline
   env.toUpperAscii().replace('.', '_').getEnv
 
-proc invokeRest(httpMethod: HttpMethod; api: string; body = ""): Response =
-  let http = newHttpClient()
-  defer: close http
-  result = http.request(getAzureEnv("System.TeamFoundationCollectionUri") &
-                        getAzureEnv("System.TeamProjectId") & api & ApiVersion,
-                        httpMethod,
-                        $body,
-                        newHttpHeaders {
-                          "Accept": "application/json",
-                          "Authorization": "Basic " & encode(':' & getAzureEnv("System.AccessToken")),
-                          "Content-Type": "application/json"
-                        })
-  if not result.code.is2xx:
-    raise newException(HttpRequestError, "Server returned: " & result.body)
-
-proc finish*() {.noconv.} =
-  if not isAzure or runId < 0:
-    return
+template getRun(): string =
+  ## Get the test run attached to this instance
+  getEnv(RunIdEnv)
 
-  try:
-    discard invokeRest(HttpPatch,
-                       ApiRuns & "/" & $runId,
-                       $ %* { "state": "Completed" })
-  except:
-    stderr.writeLine "##vso[task.logissue type=warning;]Unable to finalize Azure backend"
-    stderr.writeLine getCurrentExceptionMsg()
+template setRun(id: string) =
+  ## Attach a test run to this instance and its future children
+  putEnv(RunIdEnv, id)
 
-  runId = -1
+template delRun() =
+  ## Unattach the test run associtated with this instance and its future children
+  delEnv(RunIdEnv)
 
-# TODO: Only obtain a run id if tests are run
-# NOTE: We can't delete test runs with Azure's access token
-proc start*() =
-  if not isAzure:
-    return
-  try:
-    if runId < 0:
-      runId = invokeRest(HttpPost,
-                         ApiRuns,
-                         $ %* {
-                           "automated": true,
-                           "build": { "id": getAzureEnv("Build.BuildId") },
-                           "buildPlatform": hostCPU,
-                           "controller": "nim-testament",
-                           "name": getAzureEnv("Agent.JobName")
-                         }).body.parseJson["id"].getInt(-1)
-  except:
-    stderr.writeLine "##vso[task.logissue type=warning;]Unable to initialize Azure backend"
-    stderr.writeLine getCurrentExceptionMsg()
+template warning(args: varargs[untyped]) =
+  ## Add a warning to the current task
+  stderr.writeLine "##vso[task.logissue type=warning;]", args
+
+let
+  ownRun = not existsEnv RunIdEnv
+    ## Whether the test run is owned by this instance
+  accessToken = getAzureEnv("System.AccessToken")
+    ## Access token to Azure Pipelines
+
+var
+  active = false ## Whether the backend should be activated
+  requestBase: Uri ## Base URI for all API requests
+  requestHeaders: HttpHeaders ## Headers required for all API requests
+  results: JsonNode ## A cache for test results before uploading
+
+proc request(api: string, httpMethod: HttpMethod, body = ""): Response {.inline.} =
+  let client = newHttpClient(timeout = 3000)
+  defer: close client
+  result = client.request($(requestBase / api), httpMethod, body, requestHeaders)
+  if result.code != Http200:
+    raise newException(CatchableError, "Request failed")
+
+proc init*() =
+  ## Initialize the Azure Pipelines backend.
+  ##
+  ## If an access token is provided and no test run is associated with the
+  ## current instance, this proc will create a test run named after the current
+  ## Azure Pipelines' job name, then associate it to the current testament
+  ## instance and its future children. Should this fail, the backend will be
+  ## disabled.
+  if isAzure and accessToken.len > 0:
+    active = true
+    requestBase = parseUri(getAzureEnv("System.TeamFoundationCollectionUri")) /
+      getAzureEnv("System.TeamProjectId") / "_apis" ? {"api-version": "5.0"}
+    requestHeaders = newHttpHeaders {
+      "Accept": "application/json",
+      "Authorization": "Basic " & encode(':' & accessToken),
+      "Content-Type": "application/json"
+    }
+    results = newJArray()
+    if ownRun:
+      try:
+        let resp = request(
+          "test/runs",
+          HttpPost,
+          $ %* {
+            "automated": true,
+            "build": { "id": getAzureEnv("Build.BuildId") },
+            "buildPlatform": hostCPU,
+            "controller": "nim-testament",
+            "name": getAzureEnv("Agent.JobName")
+          }
+        )
+        setRun $resp.body.parseJson["id"].getInt
+      except:
+        warning "Couldn't create test run for Azure Pipelines integration"
+        # Set run id to empty to prevent child processes from trying to request
+        # for yet another test run id, which wouldn't be shared with other
+        # instances.
+        setRun ""
+        active = false
+    elif getRun().len == 0:
+      # Disable integration if there aren't any valid test run id
+      active = false
+
+proc uploadAndClear() =
+  ## Upload test results from cache to Azure Pipelines. Then clear the cache
+  ## after.
+  if results.len > 0:
+    try:
+      discard request("test/runs/" & getRun() & "/results", HttpPost, $results)
+    except:
+      for i in results:
+        warning "Couldn't log test result to Azure Pipelines: ",
+          i["automatedTestName"], ", outcome: ", i["outcome"]
+    results = newJArray()
+
+proc finalize*() {.noconv.} =
+  ## Finalize the Azure Pipelines backend.
+  ##
+  ## If a test run has been associated and is owned by this instance, it will
+  ## be marked as complete.
+  if active:
+    if ownRun:
+      uploadAndClear()
+      try:
+        discard request("test/runs/" & getRun(), HttpPatch,
+                        $ %* {"state": "Completed"})
+      except:
+        warning "Couldn't update test run ", getRun(), " on Azure Pipelines"
+      delRun()
 
 proc addTestResult*(name, category: string; durationInMs: int; errorMsg: string;
                     outcome: TResultEnum) =
-  if not isAzure or runId < 0:
+  if not active:
     return
+
   let outcome = case outcome
                 of reSuccess: "Passed"
                 of reDisabled, reJoined: "NotExecuted"
                 else: "Failed"
-  try:
-    discard invokeRest(HttpPost,
-                       ApiResults % [$runId],
-                       $ %* [{
-                         "automatedTestName": name,
-                         "automatedTestStorage": category,
-                         "durationInMs": durationInMs,
-                         "errorMessage": errorMsg,
-                         "outcome": outcome,
-                         "testCaseTitle": name
-                       }])
-  except:
-    stderr.writeLine "##vso[task.logissue type=warning;]Unable to log test case: ",
-                     name, ", outcome: ", outcome
-    stderr.writeLine getCurrentExceptionMsg()
+
+  results.add(%* {
+      "automatedTestName": name,
+      "automatedTestStorage": category,
+      "durationInMs": durationInMs,
+      "errorMessage": errorMsg,
+      "outcome": outcome,
+      "testCaseTitle": name
+  })
+
+  if results.len > CacheSize:
+    uploadAndClear()

testament/testament.nim

@@ -47,6 +47,9 @@ Options:
   --backendLogging:on|off   Disable or enable backend logging. By default turned on.
   --megatest:on|off         Enable or disable megatest. Default is on.
   --skipFrom:file           Read tests to skip from `file` - one test per line, # comments ignored
+
+On Azure Pipelines, testament will also publish test results via Azure Pipelines' Test Management API
+provided that System.AccessToken is made available via the environment variable SYSTEM_ACCESSTOKEN.
 """ % resultsFile
 
 type
@@ -606,6 +609,7 @@ proc main() =
   os.putEnv "NIMTEST_COLOR", "never"
   os.putEnv "NIMTEST_OUTPUT_LVL", "PRINT_FAILURES"
 
+  azure.init()
   backend.open()
   var optPrintResults = false
   var optFailing = false
@@ -656,8 +660,6 @@ proc main() =
         quit Usage
     of "skipfrom":
       skipFrom = p.val.string
-    of "azurerunid":
-      runId = p.val.parseInt
     else:
       quit Usage
     p.next()
@@ -670,7 +672,6 @@ proc main() =
   of "all":
     #processCategory(r, Category"megatest", p.cmdLineRest.string, testsDir, runJoinableTests = false)
 
-    azure.start()
     var myself = quoteShell(findExe("testament" / "testament"))
     if targetsStr.len > 0:
       myself &= " " & quoteShell("--targets:" & targetsStr)
@@ -679,8 +680,6 @@ proc main() =
 
     if skipFrom.len > 0:
       myself &= " " & quoteShell("--skipFrom:" & skipFrom)
-    if isAzure:
-      myself &= " " & quoteShell("--azureRunId:" & $runId)
 
     var cats: seq[string]
     let rest = if p.cmdLineRest.string.len > 0: " " & p.cmdLineRest.string else: ""
@@ -706,16 +705,14 @@ proc main() =
         progressStatus(i)
         processCategory(r, Category(cati), p.cmdLineRest.string, testsDir, runJoinableTests = false)
     else:
-      addQuitProc azure.finish
+      addQuitProc azure.finalize
       quit osproc.execProcesses(cmds, {poEchoCmd, poStdErrToStdOut, poUsePath, poParentStreams}, beforeRunEvent = progressStatus)
   of "c", "cat", "category":
-    azure.start()
     skips = loadSkipFrom(skipFrom)
     var cat = Category(p.key)
     p.next
     processCategory(r, cat, p.cmdLineRest.string, testsDir, runJoinableTests = true)
   of "pcat":
-    azure.start()
     skips = loadSkipFrom(skipFrom)
     # 'pcat' is used for running a category in parallel. Currently the only
     # difference is that we don't want to run joinable tests here as they
@@ -730,7 +727,6 @@ proc main() =
     p.next
     processPattern(r, pattern, p.cmdLineRest.string, simulate)
   of "r", "run":
-    azure.start()
     # at least one directory is required in the path, to use as a category name
     let pathParts = split(p.key.string, {DirSep, AltSep})
     # "stdlib/nre/captures.nim" -> "stdlib" + "nre/captures.nim"
@@ -745,8 +741,8 @@ proc main() =
   if optPrintResults:
     if action == "html": openDefaultBrowser(resultsFile)
     else: echo r, r.data
+  azure.finalize()
   backend.close()
-  if isMainProcess: azure.finish()
   var failed = r.total - r.passed - r.skipped
   if failed != 0:
     echo "FAILURE! total: ", r.total, " passed: ", r.passed, " skipped: ",