Skip to content

Make report templates resilient to typed-model null defaults#1644

Merged
mvdbeek merged 2 commits into
galaxyproject:masterfrom
mvdbeek:fix-report-template-none-iteration
May 22, 2026
Merged

Make report templates resilient to typed-model null defaults#1644
mvdbeek merged 2 commits into
galaxyproject:masterfrom
mvdbeek:fix-report-template-none-iteration

Conversation

@mvdbeek
Copy link
Copy Markdown
Member

@mvdbeek mvdbeek commented May 21, 2026

Summary

Two report templates in planemo/reports/ made assumptions about the JSON test-report shape that no longer hold after a0d765fd57 ("Add structured CLI and output schemas") introduced typed Pydantic models. merge_test_reports now round-trips reports through PlanemoTestReport.model_validate(...).model_dump(mode=\"json\"), and mode=\"json\" serialises unset Optional fields as explicit null instead of omitting them. Templates that previously relied on Jinja's silent-empty behaviour for Undefined attribute lookups break when they receive real None values.

This PR fixes two distinct user-visible symptoms of that shape mismatch:

  1. report_markdown.tpl crashes rendering any report that contains a passing test.
    The {% for problem in test.data.output_problems %} loop sat outside its {% if test.data.output_problems %} guard. output_problems is Optional[list[str]] = None, so post-round-trip it is null for every test that didn't produce output problems (including all passing tests), and Jinja raises TypeError: 'NoneType' object is not iterable. Symptom in the wild: planemo test_reports after planemo merge_test_reports crashes for any successful workflow test (e.g. https://github.com/galaxyproject/iwc/actions/runs/26224703416/job/77168877129). Fix: move the for inside the existing if.

  2. report_junit.tpl / report_xunit.tpl silently hide the real failure reason for tests that errored before a job was created.
    Both templates gated on {% if 'job' in testcase.data %} to choose between a per-job <failure> (with exit code + job dump) and a per-test fallback <failure message=\"{{ testcase.data.execution_problem }}\">. Post-round-trip the job key is always present (as None for execution-failed tests), so the guard is always true and the fallback branch is unreachable. The resulting JUnit/xUnit reports for such failures show Tool exit code: with no exit code and no message instead of the actual execution_problem — silently broken in CI dashboards (Jenkins, GitHub Actions, etc.). Fix: replace 'job' in testcase.data with testcase.data.job, which is truthy on a populated dict and falsy on None — correct for both pre- and post-round-trip JSON shapes.

Both fixes target the templates rather than actions.py:70/82's model_dump(mode=\"json\") call, because changing the dump options (e.g. exclude_none=True) would alter the on-disk JSON shape for every consumer of planemo's report files.

Audit of remaining templates

For completeness I also audited the other templates under planemo/reports/:

Template Verdict
report_text.tpl Safe — {% for %} properly nested inside {% if test.data.output_problems %}.
report_markdown_minimal.tpl Safe — only iterates raw_data.tests and a literal dict.
xunit.tpl Out of scope — different data shape, used by cmd_shed_diff / cmd_shed_update.
macros.tmpl Theoretical risk: render_steps(steps), render_invocation_messages(messages), render_invocation_details(details) iterate args without None-guards. Externally gated by {% if test.data.invocation_details %}, and the inner .steps / .messages / .details come from Galaxy's invocation API (typed elsewhere). No reproduction in the wild — leaving alone.

Test plan

  • pytest tests/test_cmd_test_reports.py — 8 pass (6 pre-existing + 2 new regression tests).
  • Each new regression test verified to fail in isolation without its corresponding template fix (git stash of the template change → test fails with the exact symptom described above).
  • Pre-commit (black, isort, ruff) clean on both commits.

mvdbeek added 2 commits May 21, 2026 15:09
`report_markdown.tpl` iterated `test.data.output_problems` outside its
guarding `{% if %}`, relying on Jinja's silent-empty behaviour for
Undefined attribute lookups when the field was absent from the dict.

After `a0d765fd57` ("Add structured CLI and output schemas") introduced
typed Pydantic models, `merge_test_reports` round-trips reports through
`PlanemoTestReport.model_validate(...).model_dump(mode="json")`. The
default `mode="json"` dump serialises unset Optional fields as explicit
`null`, so `output_problems` is now `None` in the merged JSON instead of
absent. Jinja then raises `TypeError: 'NoneType' object is not iterable`
when rendering any test that didn't produce output problems (including
all passing tests).

Move the `{% for %}` inside the existing `{% if %}` so the loop only
runs when there are problems to render, and add a regression test that
generates a markdown report from a passing test with explicit
`output_problems: null`.
`report_junit.tpl` and `report_xunit.tpl` gated on
`{% if 'job' in testcase.data %}` to decide whether to emit a
per-job <failure>/<error> (with exit code + job dump) or fall back to
a per-test <failure>/<error> message built from `execution_problem`.

After `a0d765fd57` introduced typed Pydantic models, `merge_test_reports`
round-trips reports through `model_dump(mode="json")`, which serialises
unset Optional fields as explicit `null`. The `job` key is therefore
always present in `testcase.data` -- as `None` for tests that errored
before a job was created -- so the guard always succeeds and the
execution_problem fallback branch became unreachable. The resulting
JUnit/xUnit reports for such failures show "Tool exit code: " with no
exit code and no message, hiding the real failure reason from CI
dashboards.

Replace `'job' in testcase.data` with `testcase.data.job`, which is
truthy on a populated dict and falsy on None, restoring the intended
two-branch behaviour for both pre- and post-round-trip JSON shapes.
Copy link
Copy Markdown
Collaborator

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I do not understand how a0d765f can be the source of the problem. The commit is not yet in a release, or?

Comment thread planemo/reports/report_junit.tpl
Comment thread planemo/reports/report_xunit.tpl
@bernt-matthias
Copy link
Copy Markdown
Collaborator

Looks good to me, but I do not understand how a0d765f can be the source of the problem.

I see. There is "only" a tag, but no release.

Copy link
Copy Markdown
Collaborator

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation

@mvdbeek mvdbeek merged commit f4a5257 into galaxyproject:master May 22, 2026
15 checks passed
@jmchilton
Copy link
Copy Markdown
Member

Ouch - sorry for breaking this and thank you for the tests!

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.

3 participants