calendar: add snapshot test for Date::try_add_with_options (clean version) #7226

SATVIKsynopsis · 2025-11-06T18:15:12Z

This PR adds a snapshot test for Date::try_add_with_options to ensure that date addition using DateDuration behaves as expected and produces stable results.

It is a clean recreation of the previous PR (#7221), which was linked to:

Fixes Add snapshot test of Date add/until #7215
Related to Improve efficiency of date add/until #7077

The earlier PR accidentally included unintended changes across many files after a rebase issue.
This one only contains the intended snapshot test additions under components/calendar/tests/.

Manishearth · 2025-11-06T18:17:03Z

This still changes a lot of files that it shouldn't.

Please verify your PRs before making them.

Are you using AI to make these? I think we're giving you the same feedback multiple times.

SATVIKsynopsis · 2025-11-06T18:21:13Z

The 3rd last commit which i did did all those changes you asked for perfectly. but then some workflows were failing. so I then again ran the commands and then did git add . for that purpose.

SATVIKsynopsis · 2025-11-06T18:27:48Z

I see now - I will ensure my subsequent PRs only contain the file change that was intended.

I am not using AI to create code- but I have been learning and validating everything manually, using guidance to understand how to do that better.

I will tidy this up in due course and submit a more focused PR.

Manishearth · 2025-11-06T18:34:53Z

Please use one PR instead of opening new PRs every time.

SATVIKsynopsis · 2025-11-06T18:59:43Z

Thank you for your explanation!
From now on, I will keep updates within one PR and push commits there, instead of creating new PRs.
Thanks for clarifying this, as I will clean this branch and update the same PR.

…endars and durations

SATVIKsynopsis · 2025-11-07T12:58:24Z

I did a rebase onto the latest main branch and confirmed the only changes are in the snapshot test files under components/calendar/tests/. I also ran cargo fmt and clippy locally with no warnings.

The snapshot tests are working and are ready for review. Also, thanks for the feedback earlier — I've ensured this is PR is only on the tests I intended to cover.

SATVIKsynopsis · 2025-11-08T19:19:56Z

Hi Manishearth
Thanks again for your earlier feedback — I’ve made sure this PR now only includes the necessary file changes and verified everything locally before pushing.

Just to clarify, I’m not using AI-generated code; I’ve been writing and testing these changes manually but was still learning the correct workflow for your repo.
I really appreciate your patience and guidance — it’s helped me understand how to structure contributions properly.

Manishearth

I think I would prefer the output snapshot to be a single file with all of the individual snapshots contained in it.

You can do this by collecting up a big string.

Manishearth · 2025-11-10T21:49:37Z

components/calendar/tests/snapshots/date_addition_snapshot__2023_1_1_Buddhist.snap

+expression: "(format!(\"Start: {:?}-{:?}-{:?}\", y, m, d),\nformat!(\"Calendar: {:?}\", cal_kind),\nformat!(\"Duration added: {:?}\", duration),\nformat!(\"Resulting ISO date: {:?}\", result_iso))"
+---
+(
+    "Start: 2023-1-1",


suggested format so that this fits in one line:

y-m-d (Buddhist) + DateDuration {....} = Date(y-m-d, ....)

Though I'm not sure if we should try and fit it on one line

Got it — thank you for the suggestion

I’ll modify the test to collect all individual snapshot outputs into a single string and assert once, as you suggested.

I’ll also experiment with the one-line format you mentioned to see if it remains readable.

I’ll push the updated version shortly.

…gle readable snapshot

SATVIKsynopsis · 2025-11-10T23:02:24Z

I combined all the earlier individual snapshot tests into a single, more readable snapshot that loops through multiple ISO dates, calendar types (Iso, Gregorian, Buddhist), and different duration combinations. I have also removed the old redundant snapshot files to keep the test cleaner and easier to maintain. Hope this resolves the issue.

Manishearth · 2025-11-11T00:00:41Z

components/calendar/tests/date_addition_snapshot.rs

+    let calendars = vec![
+        AnyCalendarKind::Iso,
+        AnyCalendarKind::Gregorian,
+        AnyCalendarKind::Buddhist,


issue: please include all calendars here.

Manishearth · 2025-11-11T00:02:50Z

components/calendar/tests/date_addition_snapshot.rs

+    for (y, m, d) in &iso_dates {
+        for duration in &durations {
+            for cal_kind in &calendars {
+                let cal = AnyCalendar::new(*cal_kind);


Since this is now just outputting a single file, instead of trying to collapse everything in one line, perhaps we can generate a nested list? Something like

2025-12-31

Buddhist (Date (...))

add(DateDuration {...}): Date(...)

add(DateDuration {...}): Date(...)

add(DateDuration {...}): Date(...)

Iso

...

Can we actually produce a nested list instead of syntax heavy output?

I want this to be relatively readable, this is currently very messy

I’ve reformatted the snapshot output into a nested list structure for better readability.

Each ISO start date now groups all calendars, and each calendar lists the durations and their resulting ISO dates in an indented format.

Hope this is the output you expected.

SATVIKsynopsis

I’ve restructured the test output to generate a nested snapshot format as suggested.

The results are now grouped by start date → calendar → applied durations, so it’s much easier to inspect the behavior across different calendars for each date.

This structure also keeps the snapshot compact while still covering all possible combinations.

Manishearth · 2025-11-11T17:43:41Z

components/calendar/tests/date_addition_snapshot.rs

                date.try_add_with_options(*duration, opts)
                    .expect("Addition should succeed");

                let result_iso = date.to_iso();


We should be outputting the result in that calendar, not in ISO.

Manishearth · 2025-11-11T17:44:36Z

components/calendar/tests/date_addition_snapshot.rs

+        writeln!(&mut output, "{}-{}-{}", y, m, d).unwrap();

        for cal_kind in &calendars {
+            writeln!(&mut output, "  {:?}", cal_kind).unwrap();


nit: let's compute date here and include it in the output, since the per-calendar date will be different

I’ve updated the test to output results in their respective calendars instead of converting to ISO. Each calendar section now includes both its localized starting date and all resulting dates within that same calendar.

Manishearth

I think this test looks better now.

Potentially would be nice to have a more compact representation of per-calendar dates instead of the Debug output but that's a change that can be figured out later.

@sffc are there cases you'd like to see covered here?

SATVIKsynopsis · 2025-11-11T18:58:41Z

Glad the structure looks good now.
I agree, a more compact representation would make it cleaner once the API supports it. Happy to adjust that later when ready.

SATVIKsynopsis requested review from Manishearth and sffc as code owners November 6, 2025 18:15

SATVIKsynopsis mentioned this pull request Nov 6, 2025

calendar: add snapshot test for Date::try_add_with_options #7221

Closed

SATVIKsynopsis force-pushed the clean-snapshot-tests branch from 6b4a691 to e566546 Compare November 7, 2025 12:21

calendar: add snapshot test for Date::try_add_with_options across cal…

5afd696

…endars and durations

SATVIKsynopsis force-pushed the clean-snapshot-tests branch from e566546 to 5afd696 Compare November 7, 2025 12:43

Merge branch 'main' into clean-snapshot-tests

19e5bfb

Manishearth reviewed Nov 10, 2025

View reviewed changes

SATVIKsynopsis added 2 commits November 10, 2025 22:54

Combine date addition tests across calendars and durations into a sin…

05490d9

…gle readable snapshot

minor change

deee5e9

Manishearth reviewed Nov 11, 2025

View reviewed changes

Added nested viewing in snapshot and added all the calendars

dddef8c

SATVIKsynopsis commented Nov 11, 2025

View reviewed changes

improved readibility of snapshot

f75d28f

Manishearth reviewed Nov 11, 2025

View reviewed changes

SATVIKsynopsis added 2 commits November 11, 2025 18:02

nitpick changes

d76bf7a

date changes

be01a4a

Manishearth reviewed Nov 11, 2025

View reviewed changes

Merge branch 'main' into clean-snapshot-tests

0dc213f

calendar: add snapshot test for Date::try_add_with_options (clean version) #7226

Are you sure you want to change the base?

calendar: add snapshot test for Date::try_add_with_options (clean version) #7226

Uh oh!

Conversation

SATVIKsynopsis commented Nov 6, 2025

Uh oh!

Manishearth commented Nov 6, 2025

Uh oh!

SATVIKsynopsis commented Nov 6, 2025

Uh oh!

SATVIKsynopsis commented Nov 6, 2025

Uh oh!

Manishearth commented Nov 6, 2025

Uh oh!

SATVIKsynopsis commented Nov 6, 2025

Uh oh!

SATVIKsynopsis commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SATVIKsynopsis commented Nov 8, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SATVIKsynopsis commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SATVIKsynopsis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

SATVIKsynopsis commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SATVIKsynopsis commented Nov 7, 2025 •

edited

Loading