Skip to content

Unit tests for layer_combination.py#41

Merged
scottmello37 merged 7 commits intomainfrom
test/layer_combo_tests
Feb 4, 2026
Merged

Unit tests for layer_combination.py#41
scottmello37 merged 7 commits intomainfrom
test/layer_combo_tests

Conversation

@scottmello37
Copy link
Collaborator

This PR adds a set of unit tests for layer_combination.py. The tests cover the geometry and PFA dimensionality detection, NaN handling in the prepare_for_combination and do_voter_veto functions, veto and modified-veto behavior, and basic end-to-end do_voter_veto flows for both 2D and 3D cases.

@castelao , I would love your thoughts on this test style. I tried to target the tests around just the most important parts of each function.

@scottmello37 scottmello37 requested a review from castelao January 22, 2026 00:46
@scottmello37 scottmello37 self-assigned this Jan 22, 2026
castelao
castelao previously approved these changes Feb 2, 2026
Copy link
Member

@castelao castelao left a comment

Choose a reason for hiding this comment

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

@scottmello37 , this looks great! Thanks for doing this. I particularly liked that you're taking advantage of fixtures to avoid redundancies in the tests and also that you don't try to cover many things in a single test. It's easier to take advantage of foccused tests.

I suggest you use more docstrings in the functions. You don't need to do it complete as we would do for a regular function, but just some hints on the concept of that test and what to expect. The limit here is only your time, otherwise, add as much information as you think useful without stalling your production.

The VoterVeto covers almost half of the module, and I imagine that eventually we will add more tests. What about istolating it in its own module? What about transform test_layer_combination in a module (directory with __init__.py inside it) and distribute the current tests in test_core.py (or test_misc.py or whatever you prefer) plus test_voter_veto.py?

…developers per Gui's comments. Added comments in each section describing the functions being tests. Added docstrings in each testing function to explain them concisely.
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 22.64%. Comparing base (24302ea) to head (956eef8).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   15.58%   22.64%   +7.06%     
==========================================
  Files          18       18              
  Lines        2804     2804              
  Branches      434      434              
==========================================
+ Hits          437      635     +198     
+ Misses       2340     2126     -214     
- Partials       27       43      +16     
Flag Coverage Δ
unittests 22.64% <ø> (+7.06%) ⬆️

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.

@scottmello37
Copy link
Collaborator Author

@castelao, thank you for reviewing this! I have added documentation according to your guidance in 956eef8. I plan to add the do_voter_veto tests to their own testing module as you have recommended in a subsequent PR. If you do not mind doing a final review/approval on this one, I will finalize and merge.

Copy link
Member

@castelao castelao left a comment

Choose a reason for hiding this comment

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

This looks great!

@scottmello37 scottmello37 merged commit 44e5d0d into main Feb 4, 2026
6 checks passed
@scottmello37 scottmello37 deleted the test/layer_combo_tests branch February 4, 2026 18:38
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