Skip to content

Fix mctest TypeError on re-runs: displaytime never restored from JSON - #2624

Merged
willend merged 1 commit into
mccode-dev:mainfrom
mads-bertelsen-agentic:mctest-displaytime-none-fix
Sep 3, 2026
Merged

Fix mctest TypeError on re-runs: displaytime never restored from JSON#2624
willend merged 1 commit into
mccode-dev:mainfrom
mads-bertelsen-agentic:mctest-displaytime-none-fix

Conversation

@mads-bertelsen-agentic

Copy link
Copy Markdown
Contributor

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:

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= 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 Powder n and single crystal minorfixes #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.

Note: get('displaytime') used as it won't fail if the field is absent on an old test.


Declaration of use of AI-tools

  • Please add a checkmark here if you used AI-tools during the work for this contribution
  • Furter, please describe how / where and for what the tools were used:

Development OS / boundary conditions

Please describe what OS you developed and tested your additions on, and if any special dependencies are required:
OS X 14.6.1 (23G93)


PR Checklist for contributing to McStas/McXtrace

  • My work touches the code-generator in mccode/src

    • I have added reasoning and documentation for the change through an ADR record in our GRAMMAR section
    • I am attaching test output in the comments
  • My work touches / adds to the runtime lib code (.c,.h etc in multiple locations

    • I am have added reasoning and documentation for the change below
    • I am attaching test output in the comments
  • My PR is meant to fix a specific, existing issue

  • My contribution contains something else

    • Explanation is added in free form text above or below the checklist

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 mccode-dev#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 mccode-dev#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.

@willend willend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, dumb detail. Thanks for the PR, let's just get it merged....

@willend
willend merged commit 8ad970a into mccode-dev:main Sep 3, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants