Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
IsakNaslundBh opened this issue Aug 29, 2024 · 1 comment · Fixed by #3410
Assignees
Labels
type:bug Error or unexpected behaviour type:feature New capability or enhancement type:test-script Creation of unit test required

Comments

@IsakNaslundBh
Copy link
Contributor

IsakNaslundBh commented Aug 29, 2024

Definition of the test :

In the refactoring to remove duplicate methods done in BHoM/Test_Toolkit#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

  • Add boolean options for static fields and properties to the base config
  • Make use of the booleans in methods in the diffing engine
  • Set the value to include to false for the config sent by the Test_Toolkit for CheckTest

2

  • Replicate hte previous behaviour of the method in Test_Toolkit that always rules out static properties
@IsakNaslundBh IsakNaslundBh added type:bug Error or unexpected behaviour type:feature New capability or enhancement type:test-script Creation of unit test required labels Aug 29, 2024
@IsakNaslundBh
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour type:feature New capability or enhancement type:test-script Creation of unit test required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants