Skip to content

Fix obsolete warnings in generated test code #337

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

Closed
angularsen opened this issue Dec 7, 2017 · 7 comments
Closed

Fix obsolete warnings in generated test code #337

angularsen opened this issue Dec 7, 2017 · 7 comments

Comments

@angularsen
Copy link
Owner

Tons of warnings about using the obsoleted Equals() method or == and != operators in test code.
This can and should be rewritten to the overload that takes a delta/error argument.

@dutts
Copy link
Contributor

dutts commented Dec 9, 2017

I don't mind taking a look at this @angularsen, I've used your library quite a lot in the past and so happy to help. What precision would you want the doubles to be compared to? I assume I can then remove the Obsolete attribute if we're comparing to a given precision?

@angularsen
Copy link
Owner Author

Awesome @dutts , that would be appreciated.

About precision, I think it depends a bit on what unit is being tested. However, we already dealt with this problem in tests by introducing per-unit tolerances in each quantity's test base class that can be overridden in the quantity test class when needed. The default is 1e-5 if I recall correctly, and some units that such as Teaspoon could not pass the tests with those tolerances due to a large difference to the base unit, so those set different tolerances in order to pass the tests. I think you should be able to access and use those same tolerances for these equality tests.

@dutts
Copy link
Contributor

dutts commented Dec 10, 2017

Ok I see what you're talking about. Those tolerances are currently in the TestBase classes in the Tests project. I think I'm going to run into some circular dependency issues if I try to reference the Test project from the main unless I'm missing something so should I move the tolerances into the main project to be used by both?

@angularsen
Copy link
Owner Author

angularsen commented Dec 10, 2017

Sorry, I was probably not clear.

As an example, Length.g.cs has this new method: public bool Equals(Length other, Length maxError)

We have obsoleted the "default" equals method that does not specify a maxError, and we want users to call this new method instead. The only thing remaining is to also call this method from our tests, since we get a lot of build warnings on generated test code using the default equals methods. Instead, the test code should specify the max error they allow to consider two values equal. We probably can't just specify 0.0000001 or some other fixed small value, since some units are a lot smaller/larger than the base unit and can easily accumulate fairly large errors in the internal base unit representation. This is why I propose for Length tests to pass in the CentimeterTolerance, MeterTolerance etc. for tests that call Equals on those units.

Hope this made things a bit more clear, if not please ask :)

@dutts
Copy link
Contributor

dutts commented Dec 11, 2017

Is this the sort of thing you had in mind @angularsen?

dutts@fb9350e

@angularsen
Copy link
Owner Author

Yep, looks about right to me, as long as the tests actually do pass?

@angularsen
Copy link
Owner Author

Fixed by #341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants