Cop idea: Prefer `#change` over `#round` for Time/DateTime

[ydakuka]

Describe the solution you'd like

# bad
expect(user.created_at).to eq Time.now.utc.round

# good
expect(user.created_at).to eq Time.now.utc.change(nsec: 0)

Rubocop

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop -V
1.56.2 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 2.7.8) [x86_64-linux]
  - rubocop-capybara 2.18.0
  - rubocop-factory_bot 2.23.1
  - rubocop-performance 1.19.0
  - rubocop-rails 2.21.0
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.24.0
  - rubocop-thread_safety 0.5.1

[pirj]

‘change’ is a Rails thing? And it’s probably a good recommendation for Rails code in general, not just specs?

Would ‘round’ be always identical to ‘change’ (i mean floor/ceil)?

Good to move this to rubocop-rails?

[ydakuka]

‘change’ is a Rails thing?

Yes, it should be a RSpec/Rails cop.

And it’s probably a good recommendation for Rails code in general, not just specs?

Perhaps, but personally I had such problems only in specs.

Would ‘round’ be always identical to ‘change’ (i mean floor/ceil)?

No,

irb(main):002:0> Time.parse('2023-09-11 16:01:15.497457202 UTC').round.to_i
=> 1694448075
irb(main):001:0> Time.parse('2023-09-11 16:01:15.517457202 UTC').round.to_i
=> 1694448076

irb(main):003:0> Time.parse('2023-09-11 16:01:15.497457202 UTC').floor.to_i
=> 1694448075
irb(main):004:0> Time.parse('2023-09-11 16:01:15.517457202 UTC').floor.to_i
=> 1694448075

irb(main):005:0> Time.parse('2023-09-11 16:01:15.497457202 UTC').ceil.to_i
=> 1694448076
irb(main):006:0> Time.parse('2023-09-11 16:01:15.517457202 UTC').ceil.to_i
=> 1694448076

I think if rounding affects the result, we need to use Timecop or look for problems in the code.

Good to move this to rubocop-rails?

Yep.

[pirj]

I see, thanks for this research.

With what you say, it seems that indeed, this cop makes more sense in Rails RSpec specs.

I recall that the the default timestamp nanosecond precision varies across DBs, and it was discarding nanoseconds in MySQL until recently. But how this is actually done? Does it round down at milliseconds?

How does it even work if user.created_at has millisecond precision, and the result of ‘round’ has whole seconds precision?

With this cop our ultimate goal is to avoid flakiness, right?

[ydakuka]

Does it round down at milliseconds?

No,

For PostgreSQL:

When Rails models are saved to the database, any timestamps they have are stored using a type in PostgreSQL called timestamp without time zone, which has microsecond resolution—i.e., six digits after the decimal. So when 1577987974.6472975 is sent to PostgreSQL, it truncates the last digit of the fractional part and instead saves 1577987974.647297.

Reference: https://www.toptal.com/ruby-on-rails/timestamp-truncation-rails-activerecord-tale#the-cause

For MySQL:

Rails will remove fractional part if mysql adapter does not support precision.

If precision is not set then fractional part gets stripped and date is not changed.

Reference: https://www.bigbinary.com/blog/rails-5-handles-datetime-with-better-precision

If mysql adapter supports precision, rails will drop nanoseconds:

create_table "users", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", force: :cascade do |t|
  t.datetime "updated_at", precision: 6, null: false
end

irb(main):001:0> ActiveRecord::Base.connection.select_value('SELECT version()')
=> "10.4.30-MariaDB-1:10.4.30+maria~ubu2004-log"
irb(main):002:0> ActiveRecord::Base.connection.execute("UPDATE `users` SET `updated_at` = '2016-01-18 23:59:59.999999999' WHERE `users`.`id` = 1")
=> nil
irb(main):003:0> User.find(1).updated_at.utc
=> 2016-01-18 23:59:59.999999 UTC

[ydakuka]

Also rails drops microseconds in job argument assertions

https://github.com/rails/rails/pull/35713

[ydakuka]

How does it even work if user.created_at has millisecond precision, and the result of ‘round’ has whole seconds precision?

I don't get your question.

[pirj]

user.created_at - floating point, 6 to nine digits of precision tTime.now.utc.round - integer, zero precision

Chances of ‘eq’ to match are one in a million.

Why do we even need this cop then?

[ydakuka]

The main idea of this cop is to prevent skipping precision for datetime fields. I believe that the test above should be written more reliably.

# bad, another example
expect(user.created_at.to_i).to eq Time.now.utc.to_i

P.S. I think the old problem is no longer relevant.

[pirj]

Could ‘change’ be considered as an option to write time comparisons more reliably?

I suggest starting from scratch, getting examples of unreliable specs, and finding a common solution we could suggest everyone to use.

This seems related and a good starting point https://github.com/rails/rails/issues/38831