From 73d55060f4a3b24f57f34e095dfe22bc43be9c7e Mon Sep 17 00:00:00 2001
From: mads-bertelsen-agentic
 <301266180+mads-bertelsen-agentic@users.noreply.github.com>
Date: Thu, 3 Sep 2026 10:26:43 +0200
Subject: [PATCH] Fix mctest TypeError on re-runs: displaytime never restored
 from JSON

mctest aborts a whole test run with

    TypeError: %d format: a real number is required, not NoneType

in the "Display OK/FAILED" status line whenever it re-enters an
instrument test dir that already contains results from an earlier run.
Reported by users on PR #2622 (crash at the "Display FAILED" line).

How it happens:
- If the per-instrument test dir already exists - e.g. a re-run with
  the same --testdir and --uid, which is exactly how the CI workflows
  invoke mctest - shutil.copytree fails and mctest falls back to
  InstrExampleTest.load() to rebuild the test objects from the JSON
  files saved by the previous run.
- load() restored every stored field except "displaytime", leaving the
  test object with displayed=<saved value> but displaytime=None. The
  "binary already exists" branch of the compile loop skips the
  mcdisplay step as well, so displaytime is never (re)measured.
- The run/status loop then formats test.displaytime with %d, which
  raises TypeError and kills the run before any results are written.
The "Display FAILED" variant (the one seen on PR #2622) occurs when
the first run recorded displayed=false, e.g. because mcdisplay hit the
60 s --displaymax timeout - seen for MCPL instruments such as
ESS_BEER_MCPL / ESS_KVASIR / ESS_butterfly_MCPL_test.

Recreate the error (before this fix):
    mctest --testdir tmp --instr ESS_KVASIR -n 1e4 --uid reproA
    # 1st run: SUCCESS, saves results JSON
    mctest --testdir tmp --instr ESS_KVASIR -n 1e4 --uid reproA
    # 2nd run: "WARNING: Skipped ESS_KVASIR test - did ... exist
    # already??" followed by the TypeError traceback above
The FAILED-branch variant is forced by making the display time out in
the first run, e.g. adding --displaymax 0.

Fix:
- InstrExampleTest.load() now restores displaytime from the saved JSON
  (via obj.get, so JSON files written without the key still load).
- The Display OK/FAILED status lines fall back to 0 s when displaytime
  is None, which also covers a binary present without saved display
  time (e.g. a first run interrupted between compile and saving).

Verified: both re-run scenarios above now complete with SUCCESS, and
fresh mctest runs are unaffected.
---
 tools/Python/mctest/mctest.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/Python/mctest/mctest.py b/tools/Python/mctest/mctest.py
index eb0db101e..3a16ccf32 100644
--- a/tools/Python/mctest/mctest.py
+++ b/tools/Python/mctest/mctest.py
@@ -123,6 +123,7 @@ def load(self,testnb=0):
             self.linted=obj['linted']
             self.compiled=obj['compiled']
             self.compiletime=obj['compiletime']
+            self.displaytime=obj.get('displaytime')
             self.displayed=obj['displayed']
             self.didrun=obj['didrun']
             self.runtime=obj['runtime']
@@ -419,11 +420,12 @@ def mccode_test(branchdir, testdir, limitinstrs=None, instrfilter=None, compfilt
             logging.info(formatstr % test.instrname)
             continue
         if test.testnb <= 1:
+            displaytime = test.displaytime if test.displaytime is not None else 0
             if test.displayed:
-                formatstr = "%-" + "%ds:   Display OK (%ds)" % (maxnamelen+1, test.displaytime)
+                formatstr = "%-" + "%ds:   Display OK (%ds)" % (maxnamelen+1, displaytime)
                 logging.info(formatstr % test.instrname)
             else:
-                formatstr = "%-" + "%ds:   Display FAILED (%ds)" % (maxnamelen+1, test.displaytime)
+                formatstr = "%-" + "%ds:   Display FAILED (%ds)" % (maxnamelen+1, displaytime)
                 logging.info(formatstr % test.instrname)
 
         # runable tests have testnb > 0