Skip to content

[TEST] Add tests for TriangleDisplay.#825

Merged
henrydingliu merged 2 commits into
mainfrom
#824_display_coverage
May 21, 2026
Merged

[TEST] Add tests for TriangleDisplay.#825
henrydingliu merged 2 commits into
mainfrom
#824_display_coverage

Conversation

@genedan
Copy link
Copy Markdown
Collaborator

@genedan genedan commented May 20, 2026

Summary of Changes

Adds unit tests for core.display.py

Related GitHub Issue(s)

#824

Additional Context for Reviewers

Adds a fixture called empty_triangle because it's used multiple times.

  • I passed tests locally for both code (uv run pytest) and documentation changes (uv run jb build docs --builder=custom --custom-builder=doctest)

Note

Low Risk
Low risk: primarily adds test coverage and only changes runtime behavior by consistently raising ImportError when IPython/HTML is unavailable for heatmap().

Overview
Adds extensive unit tests for TriangleDisplay rendering and formatting, including _dimensionality, __repr__, _repr_html_, _get_format_str, semi-annual origin formatting, and heatmap() behavior.

Adjusts heatmap() to always raise ImportError (with updated message) when IPython’s HTML helper isn’t available, and adds an empty_triangle pytest fixture to reuse across tests.

Reviewed by Cursor Bugbot for commit b146e62. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.38%. Comparing base (7f53616) to head (b146e62).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
+ Coverage   86.40%   87.38%   +0.97%     
==========================================
  Files          86       86              
  Lines        4959     5200     +241     
  Branches      642      717      +75     
==========================================
+ Hits         4285     4544     +259     
+ Misses        478      464      -14     
+ Partials      196      192       -4     
Flag Coverage Δ
unittests 87.38% <100.00%> (+0.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrydingliu
Copy link
Copy Markdown
Collaborator

My first impression is that test_repr_multi test_repr_html_single test_repr_html_multi doesn't test a whole lot. Maybe they don't need to. Just wanted to ask the question of, Is there some common mode of failure that we should be testing instead?

@genedan
Copy link
Copy Markdown
Collaborator Author

genedan commented May 20, 2026

_repr_html() is responsible for printing the triangles in Jupyter. The tests you listed only check whether there is some semblance of an HTML tag in there. I'll see if we can go one step further and validate the HTML syntax without bringing in additional dependencies.

I think we should keep it at that (for now), to be sure that the Triangle is displayed. The next step would be to actually see if the values in the representation are correct, but other functions handle the data, and fixing issues with those functions are more likely if the values are incorrect.

If we actually do start seeing issues in the Jupyter output caused by _repr_html(), we can take the tests further.

@henrydingliu
Copy link
Copy Markdown
Collaborator

i'm good with that. we should circle back before we migrate all our docs to rst.

henrydingliu
henrydingliu previously approved these changes May 20, 2026
@genedan
Copy link
Copy Markdown
Collaborator Author

genedan commented May 20, 2026

Added some testing for malformed HTML. I felt like even the parsing function would need its own test, so I added that too. test_repr_multi is the same though, it's just a quick and dirty check to see if we get a summary instead of a single-dim triangle.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b146e62. Configure here.

with pytest.raises(ImportError, match=r"heatmap\(\) requires IPython\."):
raa.heatmap()

importlib.reload(display_mod)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Module reload cleanup not guaranteed on test failure

Low Severity

The cleanup importlib.reload(display_mod) calls at the end of test_heatmap_no_ipython and test_display_import_fallback_when_ipython_missing are not wrapped in try/finally. If an assertion inside the with mock.patch.dict(...) block fails, the mock.patch.dict context manager restores sys.modules, but the module's global HTML stays None because the final importlib.reload is skipped. This leaves display_mod in a dirty state, causing cascading failures in later tests like test_heatmap_render that depend on HTML being available.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b146e62. Configure here.

Copy link
Copy Markdown
Collaborator

@henrydingliu henrydingliu left a comment

Choose a reason for hiding this comment

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

love the new tests for html integrity

@henrydingliu henrydingliu merged commit b748a4d into main May 21, 2026
17 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