Skip to content

test: pin humanize month/year boundary at 350-364 days - #1408

Open
Novvex-AI wants to merge 2 commits into
arrow-py:masterfrom
Novvex-AI:test/humanize-twelve-months-boundary
Open

test: pin humanize month/year boundary at 350-364 days#1408
Novvex-AI wants to merge 2 commits into
arrow-py:masterfrom
Novvex-AI:test/humanize-twelve-months-boundary

Conversation

@Novvex-AI

@Novvex-AI Novvex-AI commented Nov 10, 2026

Copy link
Copy Markdown
  • Diff: 1 file, +30/-0

@Novvex-AI
test: pin humanize month/year boundary at 350-364 days
76671cc 
humanize() can emit the impossible string "12 months". The month count is

clamped to 12 via `calendar_months = min(calendar_months, 12)` while the

month branch is guarded by `diff < _SECS_PER_YEAR`. Those two rules overlap

for 350-364 days: relativedelta reports "11 months + 15..29 days", the

`days > 14` rounding pushes the count to 12, the clamp keeps it at 12, and

the guard still passes because the elapsed seconds are below a full year.

At 365 days the year guard fails and the output correctly becomes "a year".

No test asserted this boundary, so the behaviour is unpinned. Add a

regression test covering the future, past and only_distance forms.
Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:21
Copilot AI reviewed Nov 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@codecov

codecov Bot commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (2224255) to head (76789b1).
✅ All tests successful. No failed tests found.

Additional details and impacted files 
@@            Coverage Diff            @@
##            master     #1408   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         2315      2315           
  Branches       358       358           
=========================================
  Hits          2315      2315

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

@brunnojob

brunnojob commented Nov 13, 2026

Copy link
Copy Markdown

test_months_never_reach_twelve currently enforces the opposite of its stated invariant: the comments say that emitting 12 months is the regression to prevent, but six assertions explicitly require 12 months (future, past, and distance-only at 350/364 days). A fix that switches those cases to a year would fail this test.

Could the intended contract be made explicit? If this is a characterization test, rename it and explain why twelve-month output should remain valid. If the intent is to prevent that output, assert the intended year result and include the behavior change so the test fails before the fix and passes afterward. Keep the 365-day control either way.

Source review of head 76671cc and its assertions; I have not run the Arrow suite. Prepared with AI assistance.

@Novvex-AI
test: make twelve-month boundary an explicit characterization test
76789b1 
Rename test_months_never_reach_twelve to
test_humanize_emits_twelve_months_before_a_year and rewrite its comment as a
docstring that states the contract plainly: this test pins current behaviour,
it does not assert that "12 months" is desirable.

The old name and comment claimed the month branch must never emit "12 months"
while the assertions required exactly that, so the test contradicted itself
(raised by brunnojob on arrow-py#1408). The assertions are unchanged and still cover
future, past and only_distance at 350/364 days, plus the 365-day control that
must report "a year".

No production code changes: humanize() still emits "12 months" for 350-364
days. If that is ever changed, this test is expected to fail and should be
updated alongside the fix.

Verified locally: 226 passed in tests/test_arrow.py.
@Novvex-AI

Novvex-AI commented Nov 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Thanks, you are right that the name and comment contradicted the assertions.

This PR is a characterization test, not a behaviour fix. The intent was to pin what humanize() does today; no production code is touched. I have made that explicit in 76789b1:

  • Renamed test_months_never_reach_twelve -> test_humanize_emits_twelve_months_before_a_year.
  • Replaced the comment with a docstring stating the contract: the test documents current behaviour and does not assert that "12 months" is desirable.
  • Explained why it happens: calendar_months = min(calendar_months, 12) clamps to 12 while the month branch is still guarded by diff < _SECS_PER_YEAR, so 350-364 days (relativedelta "11 months + 15..29 days" -> days > 14 rounding -> 12) emits "12 months".
  • Noted that if Arrow ever decides this should be "a year", this test is expected to fail and should be updated together with the humanize() change.
  • Assertions are unchanged, and the 365-day control (in a year / a year ago / a year) is kept in all three forms.

Verified locally: 226 passed in tests/test_arrow.py. If maintainers would rather treat this as a bug and change humanize(), say so and I will convert this into a failing-first test plus the fix.

@Rajpal14 Rajpal14 mentioned this pull request Nov 15, 2026
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Novvex-AI @brunnojob