BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 15 forks source link

Missing skipping static properties for ObejctDifferences for when checking results of unit tests #3406

Closed IsakNaslundBh closed 2 months ago

IsakNaslundBh commented 3 months ago

Definition of the test :

In the refactoring to remove duplicate methods done in https://github.com/BHoM/Test_Toolkit/pull/474 to make use of the ObjectDifferences in the Diffing_Engine, a small detail was missed, which is that the method that was removed ruled out static properties and static fields.

The method in the Diffing_Engine does include these, which has a quite detrimental effect on comparing some system classes, so far, namely the System.Drawing.Color class, that has a significant list of other System.Drawing.Color properties, which then means that the comparison goes recursive until the max limit of properties to compare is hit.

Like color.Red.Red.Red.Red.Red...........Red.Red.Red.Red.A

To fix this, we need to either add a bool on the BaseComparisonConfig to control if static properties and fields should be included or not, or just always rule them out in the methods in the Diffing_Enigine. First option is probably better, as giving better flexibility.

Found this by realising that the Trasform UT in Graphics_Engine had started to take 10 minutes to evaluate, and realised it was the Colors that took 70ms per colour, which ofc adds up when you compare 7k vertecies.

So, to fix I see two options:

1

2

IsakNaslundBh commented 2 months ago

After discussion offline with @alelom and @pawelbaran we concluded that static properties and fields can be ruled out safely for all the cases that we can think of that we are currently using this for. Currently have a hard time even coming up with a case, for when comparing two objects of the same type, for how two static properties even theoretically could be different, and on top of that, for the usecase of this method, it quite clearly relates to the differences in instance properties.

Hence, going to action this with Option 2 above, simply ruling out static fields and properties.