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

Fixed unit tests NFUnitTestConversions #141

Merged
merged 3 commits into from
May 24, 2021

Conversation

edleno2
Copy link
Contributor

@edleno2 edleno2 commented May 24, 2021

Description

In the unit tests added some more boundary test cases to include the values where the nanoprint.c will/will-not return 'oor' for out-of-range conditions. Added a double-to-hex to allow testing of values that are too big to be tested using ToString (due to the 'oor' above) and tested that double and float min/max generate bit values in the double/float that meet the IEEE 574 standard for floating numbers. Added messages in a few test cases just to be able to tell which test is failing since line numbers in the test framework are only to the method name.

Small change in System.Convert to correctly document that UInt64 is an unsigned value - native code already overrode the value passed to the method.

Motivation and Context

These changes clean up most of the unit test failures in CoreLibrary, putting us closer to turning on unit tests as part of the build/verify process. There are still failing unit tests in other areas of coreLibrary which will be addressed in a later pull request.

How Has This Been Tested?

Tested with Win32, STM32F429I-Discovery and ESP32-WROOM-32 using CoreLibrary units test NFUnitTestConversions. For Win32 ran all unit tests for the library to confirm no additional tests were broken.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (work on Unit Tests, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Simple remark and set the IsSigned to false for the UInt64 convert method.  Just documentation since IsSigned would be set to false in the native code anyhow.

Removed and changed any tests that required the ToString() methods for double and float to handle their max values.  Most of the tests now look at the actual double/float as stored which uses the IEEE 574 format.  Hex display is used for actual asserts.

Added message lines to tests as needed to be able to tell which test was actually failing.

Fixes #715
Removed checks for hex values for some values since different platforms can calculate a double differently.  Cleaned up comments.

Fix #715
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for tackling this one. 👍🏻

@josesimoes
Copy link
Member

@edleno2 I've remove the BUG label because this is not a bug in the library code, rather in the Unit Tests. Otherwise this would be wrongly listed as a bug fix in the changelog.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adjusting those tests. Looks good to me.

@josesimoes josesimoes merged commit 9869e58 into nanoframework:develop May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants