Implement Number.InDeltaRelative

[selborsolrac]

This PR resolves #156.

Please let me know if anything needs to be changed!

[selborsolrac]

The linter is failing because I have a line that is 94 characters long, a bit over the 90 character limit. I think I've made it as short as I can while following the pattern of similar comment lines. Do you want me to try to shorten it some more or is it alright to raise the linter column limit to 94?

[gavv]

Thanks for PR! See a few comment below.

[gavv]

The linter is failing because I have a line that is 94 characters long, a bit over the 90 character limit. I think I've made it as short as I can while following the pattern of similar comment lines. Do you want me to try to shorten it some more or is it alright to raise the linter column limit to 94?

Yep, please shorten it. I understand this is a matter of taste, but I'm following this style in my projects, which allows editing code even on a smaller screen split in half vertically, like it's on one of my machines :]

[gavv]

After merging #305, TestNumber_InDelta is now table-driven. Could you please make TestNumber_InDeltaRelative table-driven too?

httpexpect/number_test.go

Line 212 in b47b582

func TestNumber_InDelta(t *testing.T) {

[github-actions]

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

[coveralls]

coverage: 95.74% (+0.09%) from 95.655% when pulling e13d9a5 on selborsolrac:156-number-indeltarelative into 537a127 on gavv:master.

[selborsolrac]

Hello,

I've implemented the requested changes as I understood them. I'm not sure I properly implemented the corner cases though. For example, I set the test to fail if any provided parameter is 0, Inf, or NaN, but I'm not sure that was your intention. For example, a corner case where the value is 0 is fine if the delta is 1 and a corner case where the delta is 0 is fine if the value is equal to n.value, so we may not want to necessarily error out in those cases. Let me know what I should further tweak!

[gavv]

So let's break down the corner cases

Given it another thought, mathematically, I think here's how it should work:

actual / expected	123	0	-inf	+inf	nan
123	true	false	false	false	invalid
0	false	true	false	false	invalid
-inf	false	false	true	false	invalid
+inf	false	false	false	true	invalid
nan	invalid	invalid	invalid	invalid	invalid

Here:

• true means that InDeltaRelative succeeds and NotInDeltaRelative fails
• false means that InDeltaRelative fails and NotInDeltaRelative succeeds
• invalid means that both of them fail

What do you think about changing formula from:

math.Abs(n.value-value) / math.Abs(n.value) > delta

to:

math.Abs(n.value-value) > math.Abs(n.value) * delta

This way zero will be handled automatically. If n.value is zero, InDeltaRelative will succeed when and only when value is zero (and delta is anything finite).

We'll still need to handle manually inf and nan. If either of values is nan, we should fail. If either of values is inf, we should succeed only if they're both infs of the same sign.

BTW we should also check that delta is non-negative, and fail with AssertUsage otherwise.

[gavv]

Hm, on the other hand, the formula I suggested will make precision worse.

Then a better option is to use the old formula, but pretend that we're using the new one by handling zero specially.

[selborsolrac]

Alright, I've made some changes to try and accommodate all of the edge cases. I think the logic and tests are correct, but please review and confirm.

Additional cases added:

• Output assert usage error if delta is negative
• Special formula if target is 0
• Add test for reference = 0
• Error out if target and/or reference are Inf

A note on the Inf values:
It appears that (+-Inf) / (+-Inf) = NaN, so we error out if the target and/or reference are +-Inf. If I'm missing something and there's a way to make this work, please let me know!

A note on the relativeDelta.String() function:
It looks like using %v formats the decimal so that it outputs the minimum number of decimal places (e.g. 1.25000 is output as 1.25). However, we can't use %v in the Stringer function because it produces error fmt.Sprintf format %v with arg rd causes recursive so I opted to use strconv to replicate the same behavior. Let me know if there's a better way to handle this!

[selborsolrac]

I think I can make the +-Inf cases work if I use the same rearranged formula used when target = 0. I'll try implementing that.

[gavv]

It seems we can combine two checks:

	if n.value == 0 {
		if math.Abs(n.value-value) > math.Abs(n.value)*delta {

	relativeDiff := math.Abs(n.value-value) / math.Abs(n.value)

	if relativeDiff > delta {

into one:

if (n.value == 0 && value != 0) || math.Abs(n.value-value) / math.Abs(n.value) > delta {
    ...
}

[gavv]

Or, generalizing it to inf too, as you mentioned above:

if ((n.value == 0 || math.IsInf(n.value, 0)) && value != n.value) || 
  math.Abs(n.value-value) / math.Abs(n.value) > delta {
    ...
}

[selborsolrac]

Hello, I've made the requested changes related to Inf numbers, let me know what you think.

The CI failed but it looks like maybe it's a transient issue that can be resolved with a retry.

[gavv]

Thanks a lot for update!

Here is a small follow-up commit: 2b1131c

I've rearranged checks a bit and changed one bit of behavior: NotInDeltaRelative now succeeds if numbers are opposite infs. Also added a few more tests.