fix: equivalent negative durations add to the same time

[cpb]

Summary

Modulo does not work as needed for negative numbers. (thanks @bradenchime for digging into that).

This PR avoids sending negative values into the modulo based parts calculating algorithm of ActiveSupport::Duration.build

Context

I want to help the next person who attempts to use ActiveSupport::Duration.build and has negative numbers slip into there in their use case.

This PR makes ActiveSupport::Duration.build(-1) not only == -ActiveSupport::Duration.build(1) but also results in the same Time when added.

I've noticed, in the closed Issues related to ActiveSupport::Duration that there is counter intuitive behaviour associated with duration math. However, because -1.second or even ActiveSupport::Duration::Scalar.new(-1) modifies Time as I expected ActiveSupport::Duration.build(-1) to, I thought: perhaps this is unexpected behaviour.

Alternatives

If it is decided to not change the behaviour of ActiveSupport::Duration.build(-1) then I humbly propose changing my last assert_equal to refute_equal and adding (at least) documentation to ActiveSupport::Duration.build that it is not expected to work with negative values, and to use Numeric#second or ActiveSupport::Duration::Scalar (for example) instead.

Observed Error Rate

This is a single sample of the slippage or error rate in the modification of Time with a ActiveSupport::Duration.build with a negative value. When I complete collecting the data for the range of whole numbers in a year, I'll be able to share a chart that illustrates the range of errors.

This will ultimately be a graph of the errors introduced in the algorithm by taking a modulo of a negative number.

time = Time.now

a = time + ActiveSupport::Duration.build(-1)
b = time + -ActiveSupport::Duration.build(1)

ActiveSupport::Duration.build((a-b).abs) # => `10 hours, 29 minutes, and 6.0 seconds`

[cpb]

If buildkite is able to reproduce the test failure on:

assert_equal expected_time, time + ActiveSupport::Duration.build(-1)

I'll add cpb@f12a0fa which provides a fix.

[cpb]

If buildkite is able to reproduce the test failure on:

assert_equal expected_time, time + ActiveSupport::Duration.build(-1)

I'll add cpb@f12a0fa which provides a fix.

activesupport (2.5) ❌
activesupport (2.6) ❌
activesupport (2.7) ❌
activesupport (3.0) ❌

[cpb]

See further discussion: https://discuss.rubyonrails.org/t/activesupport-duration-build-1-vs-activesupport-duration-build-1/79458/2

[rafaelfranca]

Instead of calling build again, can we doing the same as #43809?

[bradenchime]

For reference @rafaelfranca #43809 was put up to place-hold an alternative option for fix. Given the preference for that approach @cpb has kindly brought that work over into this PR.

I will close #43809 now.

[cpb]

@rafaelfranca if it helps, I created a PR for 7-0-stable as well

[cpb]

Let me know if you want one for main -- I'm improvising here, dunno if I'm getting ahead of myself.

[ghiculescu]

All PRs should target main, then can be backported to old versions as appropriate. Could you update this one to target main?

[cpb]

Yes! Thanks for that clarification 😊

[cpb]

@ghiculescu updated to main, renamed other PR's for backporting. Or, should I close them?

#43815

#43814

[ghiculescu]

I think just close them. Core team will decide on backports when they review this one.

[ghiculescu]

I think this needs a CHANGELOG entry too.

[cpb]

@ghiculescu Done! 🙌