Skip to content

Test PowerPoint missing text with real fixtures and shared vectors - #2594

Merged
Adam Fourney (afourney) merged 2 commits into
mainfrom
tests/pptx-missing-text
Oct 3, 2026
Merged

Adam Fourney (afourney) merged 2 commits into
mainfrom
tests/pptx-missing-text

Conversation

@afourney

Copy link
Copy Markdown
Member

The missing-text PowerPoint regressions previously replaced library properties with fabricated None values and only asserted that conversion returned non-None output. They now convert real saved presentations and verify that surrounding content survives through the shared library and CLI vectors.

  • Add four separately named fixtures copied from test.pptx: empty title/body text runs, an empty notes run, and a missing notes body frame. Preserve the original fixture and include a rebuild script.
  • Move content-preservation and forbidden-output checks into the shared vectors; remove duplicate assertions from the dedicated tests.
  • Retain small checks that the fixtures contain the intended XML conditions and that the affected slide has no spurious notes heading.

An empty <a:t/> is normalized to an empty string by python-pptx; it does not produce None shape text. The missing notes-body fixture exercises a genuinely absent notes text frame.

Validation: 49 PowerPoint tests passed before vector consolidation; after consolidation, 40 targeted vector/structural cases passed and 8 remote cases were skipped. Changed Python files formatted with Black 23.7.0; git diff --check passed.

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

The tests accurately exercise the intended real-file edge cases across structural, library, and CLI paths.

Review effort: Balanced
Findings: None

What changed in this PR

Updates PowerPoint regression tests to use real presentations and shared conversion vectors instead of fabricated None values.

Changes:

  • Adds reproducible fixtures for empty text runs and missing notes frames.
  • Validates fixture structure in dedicated PowerPoint tests.
  • Shares content-preservation assertions across library and CLI tests.
File Description
packages/​markitdown/​tests/​test_pptx.py Adds structural fixture and notes-heading checks.
packages/​markitdown/​tests/​_test_vectors.py Adds shared conversion expectations for four regressions.
packages/​markitdown/​tests/​test_files/​generate_pptx_missing_text_fixtures.py Rebuilds the regression fixtures.
packages/​markitdown/​tests/​test_files/​pptx_empty_title_run.pptx Covers an empty title run.
packages/​markitdown/​tests/​test_files/​pptx_empty_body_run.pptx Covers an empty body run.
packages/​markitdown/​tests/​test_files/​pptx_notes_empty_run.pptx Covers an empty notes run.
packages/​markitdown/​tests/​test_files/​pptx_notes_missing_body_frame.pptx Covers an absent notes body frame.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The new vectors cause eight local remote-test failures because their URLs target fixtures not yet present on main.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment on lines +75 to +79
FileTestVector(
filename="pptx_empty_title_run.pptx",
mimetype="application/vnd.openxmlformats-officedocument.presentationml.presentation",
charset=None,
url=None,
@afourney
Adam Fourney (afourney) merged commit 331519d into main Oct 3, 2026
27 checks passed
@afourney
Adam Fourney (afourney) deleted the tests/pptx-missing-text branch October 3, 2026 23:34
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