[Test] Tolerance parameter in attachmenttest needs to be one higher for integers

[demeritcowboy]

Overview

See #15066 (comment)

@seamuslee001

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

SO just reading

Thanks that seems like a good compromise to avoid touching the security fix code. But should the tolerance parameter be 2? That approxEquals function was new to me so I looked at it and the comparison is a strict less than, so the way I read it a tolerance of 1 actually means zero tolerance for integers?

And the code - it looks like we are not casting what we pass to the function - so it's receiving a number as a string - which it would treat as a float?

Wouldn't we only need this is tests do not reliable pass?

[seamuslee001]

@Eileen in this case the issue is that if a second goes by between the create and the get call then the time stamp in the URL will be different. So we are aiming to test all the other parts with assert Equals but allow for up to 1 second difference in the time stamp. But given the code I think @demeritcowboy’s suggestion of 2 for the tolerance makes sense. His point is that even if the diff between the 2 timestamps was 1 the test would fail as the assert code is only looking for things less than the tolerance not less than or equal to.

[demeritcowboy]

If it's desired and the concern is goofy type-juggling I can put up an alternate PR that uses the same idea but doesn't use approxEquals, since here it's really just one of two possible results and they're known exactly. And then it can also properly verify that it's digits and have minimal type juggling.

I'm assuming it's not worth also thinking about how in the year 2286 this test will fail again. By then it's unlikely humans, or martians, using Civi would be using something as primitive as file attachments.

[seamuslee001]

Merging based on Tim's comments in Chat https://chat.civicrm.org/civicrm/pl/m5qjmi5yk7fm8y77bxgoxaxmsa

[totten]

Couple quick comments (continuing from MM for record):

• 👍 for bumping tolerance from 1 to 2.
• 👍 for updating assertApproxEquals() to be a little more lenient (accepting "<=" rather than strictly "<")

Basically, if you're doing something that relies on approximate equality, then there's almost never a "right" number. The goal is just to get in the right ballpark. If the test-suite accepts either of those, then that's fine.