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

NFUnitTestConversions.UnitTestConvertTests.Convert are failing #715

Closed
Ellerbach opened this issue Mar 5, 2021 · 10 comments · Fixed by nanoframework/nf-interpreter#1928
Closed

Comments

@Ellerbach
Copy link
Member

Details about Problem

nanoFramework area: (mscorlib)

Description

Some of the Convert functions are failing:

  • public void Convert_Whitespace()
    • issue is with the white spaces in front and at the end of the string to convert: most likely removing them should fix the issue
  • Convert_Negative()
    • this try to convert negative numbers with non negative value types (byte, uint, ulong, ushort). That should be easy to fix by detecting if the number is negative and raising the exception as needed.
  • Convert_DoubleNormalizeNeg()
  • failing with edge case on double numbers
  • Convert_BoundaryValues()
    • failing with the Max and Min of numbers. To be explored

Screenshot

image

@SandorDobos
Copy link

This may be a duplicate of #709 or at least related to it.
The Parse methods there just wrappers around appropriate Convert methods.

@edleno2 edleno2 self-assigned this May 4, 2021
@edleno2
Copy link

edleno2 commented May 4, 2021

I'll also take a look at whether this is related to #709 - cleaning up unit tests in core library.

@edleno2
Copy link

edleno2 commented May 14, 2021

@Ellerbach @josesimoes Finally got all of my debugging capability set up. For the first set of issues: unit tests using floating point. The main issue is the the printf that is implemented is designed to be fast and small - suitable for MCU's. However, it can only handle the range of 2^-63 thru 2^63 in its ability to display. Number less than 2^-63 will produce a "ToString()" of zero, and any floats above 2^63 will get a string "oor" for out-of-range. Here's a link to the algorithm taken from nanoprint.c: http://0x80.pl/notesen/2015-12-29-float-to-string.html. Basically it shifts the internal bits into a 64 bit UINT, so you lose significant digits a 2^64 and above, and same thing happens when shifting the bits to the right on more than 2^-63. So I can change to unit tests to NOT do their compares against the boundary conditions using "ToString()" but instead to pull from the double the mantissa and exponent of the floating number and confirm that the internal storage is correct. Alternatively I could find a better float-to-string, or see if all of the compilers we use have a float-to-ascii routine. Certainly the shortest path is to just fix the unit tests and leave the core code alone. Advice?

@josesimoes
Copy link
Member

@edleno2 what is there now it's the 3rd iteration on improving and optimizing the number-to-string algorithm. The goal was (is) to have a better and more compact implementation than the one offered by newlib nano.
This last iteration I consider being very efficient.

Having said that and keeping in mind the context we are working on (embedded systems and small MCUs) at some point we have to accept (and embrace) simplifications and compromises. I find it very very acceptable and accurate enough (actually make that very) for the vast majority of use cases that one can think of, those 2^-63.

So, let's have those tests adjusted to what we have there now and move forward.

Appreciate (really) your offer to look into improving even further what we have. But I think the energy and time that you're willing to commit are better placed in new features and improving what's needing improving. Instead of squeezing an extra precision bit or shave a few cpu cycles. 😉

@edleno2
Copy link

edleno2 commented May 15, 2021

Having said that and keeping in mind the context we are working on (embedded systems and small MCUs) at some point we have to accept (and embrace) simplifications and compromises. I find it very very acceptable and accurate enough (actually make that very) for the vast majority of use cases that one can think of, those 2^-63.

@josesimoes I thought that would be the way to go so I had already made the test changes - didn't know about it being third generation, but good enough for the MCU world certainly at this point. I should have a PR ready in a day or so for the first set of changes that includes all of the double/float tests. I'd like to have you look over them for style, appropriateness of comments, direction, etc.

@josesimoes
Copy link
Member

Nice! Happy to review all that. 🙂

@edleno2
Copy link

edleno2 commented May 19, 2021

Finally have all of the UnitTestConvertTests passing on win32. Most of the problems were in the native code, some issues with the tests themselves. I'm now going to build firmware for STM32 and ESP32 and run the tests on the hardware. May take a little longer, have to learn the new devcontainer process and building firmware tends to be error prone. I did fix some parsing issues as part of doing this, but I see that there are many more left - which is what I'll work on right after I submit the PR for this issue.

@josesimoes
Copy link
Member

Brilliant! Let's have that PR. Can't wait to have all those green and finally switch on the option in the pipeline to have these as a condition for the build to pass. 😉

@edleno2
Copy link

edleno2 commented May 19, 2021

Brilliant! Let's have that PR. Can't wait to have all those green and finally switch on the option in the pipeline to have these as a condition for the build to pass. 😉

Why do I have the feeling I'll be the first to break the build with a failed unit test - reverse karma for having fixed the unit tests - LOL

@josesimoes
Copy link
Member

Fear not!! 🤣

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

Successfully merging a pull request may close this issue.

4 participants