False positives when using assert_almost_eq()

[db0]

I have noticed that my tests are working when they should be failing.

[Passed]: [Vector2(565, 600)] expected to equal [Vector2(565, 640)] +/- [(2, 2)]: Check card placed in correct global position
I have done various attempts to make it fail, but the only thing that worked was switching to assert_eq() or reducing the "jitter" to something like 0.0001.

You can download this branch of my project and replicate it on your end as well: https://github.com/db0/godot-card-gaming/tree/ControlNodeHand

I've left test_card_draw.gd with a test that should fail in test_single_card_draw()

[bitwes]

It looks like we missed some edge cases when testing vectors. It seems Godot is only using the x component in the comparison. Under the covers we aren't doing anything to specifically handle Vector2, it just checks the value against the bounds:

if(got < (expected - error_interval) or got > (expected + error_interval)):

I'm going to remove the Vector2 examples from the documentation as a quick fix for the next release (which should be out in a week or so). I'll leave this open for a little bit to see if there isn't something that should be done.

In this case, you are comparing positions so you would probably have to use assert_almost_eq on both the x and y. If you are comparing them as "vectors" then this forum thread covers how you should compare them. Namely, use .length() to compare vector size and the dot product to compare direction.

[bitwes]

Possible changes for asserrt_almost_eq and assert_almost_ne:

• Check datatypes. Should it only accept floats and ints?
• Add some logic for Vector2s, Vector3s?
  • I'm not sure this would make sense since you could not know how they are being used. Is the +/- for the length and/or the direction? Would it make sense to always treat as a position?
  • The more I type things out here the more I think checking datatypes is the right move.
• Unchanged seems like a bad idea since Godot doesn't do anything intuitive with > and <.

[db0]

why not compare x and y individually under the table?
(Just an example, don't know your code exactly...but maybe I could take a look)

assert = false  
if got.type == Vector2:
   if got.x < (expected.x - error_interval.x) and got.x > (expected.x - error_interval.x):
     if got.y < (expected.y - error_interval.y) and got.y > (expected.y + error_interval.y):
	    assert = true
else:
   if(got < (expected - error_interval) or got > (expected + error_interval)): assert = true

[db0]

I also don't think length is the correct solution either since Vector2(240,120).length() == Vector2(120,240).length() which is not accurate for positions

[db0]

I've sent a pull request to what I think is the simplest solution, a direct comparison of each component of Vector2 or Vector3. It works correctly on my end

[db0]

Also re

I'm not sure this would make sense since you could not know how they are being used. Is the +/- for the length and/or the direction? Would it make sense to always treat as a position?

I believe assert_almost_ne and assert_almost_eq should be left with the obvious comparison of float (i.e. positions) as per my merge request. The comparison for length and direction should be noted in the documentation along with examples on how to achieve it (.e.g. using assert_almost_eq on the .length() methods)

[astrale-sharp]

Hey @db0 , shouldn't you put the PR on this repo? :)

[db0]

@astrale-sharp , you mean #240?

[bitwes]

Fixed in 7.2.0, thx db0