Skip to content

fix(tests): use assert in test_n_periods so failures actually fail#744

Merged
kennethshsu merged 1 commit intocasact:mainfrom
SaguaroDev:fix/test-n-periods-assert
May 7, 2026
Merged

fix(tests): use assert in test_n_periods so failures actually fail#744
kennethshsu merged 1 commit intocasact:mainfrom
SaguaroDev:fix/test-n-periods-assert

Conversation

@SaguaroDev
Copy link
Copy Markdown

@SaguaroDev SaguaroDev commented May 7, 2026

Summary of Changes

chainladder/development/tests/test_development.py::test_n_periods ended with return xp.all(...) instead of assert xp.all(...). Two consequences:

  1. The boolean result was silently discarded, so the test did not actually fail when the equality broke — regressions in Development(n_periods=3, average='volume') were masked.
  2. Pytest 9 emits PytestReturnNotNoneWarning for any test function that returns a non-None value.

This PR replaces the single return with assert. One-line change.

Verification

  • Before fix, with -W error::pytest.PytestReturnNotNoneWarning: the test errors out on the warning.
  • After fix: pytest chainladder/development/tests/test_development.py -W error::pytest.PytestReturnNotNoneWarning35 passed, 1 xfailed, 0 PytestReturnNotNoneWarnings.
  • Mutation check: temporarily replaced one expected LDF (1.1649.999) and confirmed the test now fails with AssertionError (it previously would have passed silently).

This is purely a pytest hygiene fix and is independent of the pandas 3.0 work in #664 / #729.

Related GitHub Issue(s)

Closes #743

Additional Context for Reviewers

I grepped ^ return across test_*.py to confirm this is the only test in the package with this bug; the other 5 matches are all legitimate fixture returns.


Note

Low Risk
Low risk: changes a single test to use assert, making failures visible and eliminating pytest warnings; no production code changes.

Overview
Fixes test_n_periods in chainladder/development/tests/test_development.py to assert xp.all(...) instead of returning the boolean result.

This ensures regressions in Development(n_periods=3, average="volume") cause an actual test failure and avoids pytest warnings about non-None test returns.

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

The test ended with `return xp.all(...)`, which silently discards the
boolean result. Pytest 9 also emits PytestReturnNotNoneWarning. Replacing
`return` with `assert` makes the test enforce the expected LDFs and
clears the warning.

Closes casact#743
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.94%. Comparing base (822c052) to head (f1fd507).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #744   +/-   ##
=======================================
  Coverage   85.94%   85.94%           
=======================================
  Files          85       85           
  Lines        4924     4924           
  Branches      637      637           
=======================================
  Hits         4232     4232           
  Misses        491      491           
  Partials      201      201           
Flag Coverage Δ
unittests 85.94% <ø> (ø)

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.

@genedan
Copy link
Copy Markdown
Collaborator

genedan commented May 7, 2026

Thanks for taking this on @SaguaroDev. We have a lot of warnings in pytest, so we really appreciate any help on knocking these out.

@kennethshsu
Copy link
Copy Markdown
Collaborator

@SaguaroDev amazing!! Well done on your first PR! Welcome to the group!

@HeatherLDavis should be reaching out to you shortly if she hasn't yet, in case you are interested in joining the WG calls (there's a bit more detail on the readme).

@kennethshsu kennethshsu merged commit 943e0e2 into casact:main May 7, 2026
9 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.

test_n_periods uses return instead of assert

4 participants