-
-
Notifications
You must be signed in to change notification settings - Fork 33
Add F0 and N0 format tests for float and double ToString() #255
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
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
…file Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
|
@copilot you are supposed to improve the existing ToString() unit tests for float and double by adding new asserts which are in the reproducing code in nanoframework/Home#1650. Nothing else at this time. |
I've reverted the new comprehensive test methods I created. However, I'm unable to access nanoframework/Home#1650 to see the specific reproducing code and asserts that should be added to the existing tests. Could you please share the specific test cases or sample code from that issue so I can add the correct asserts to the existing ToString() unit tests? |
Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
|
@copilot I've fixed the URL for the issue. It should be nanoframework/Home#1650 |
Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot still incorrect. Since you can't access the issue, here's the sample code the user provided: namespace Blank } |
…999) Co-authored-by: josesimoes <1881520+josesimoes@users.noreply.github.com>
Added the specific test values from the sample code (9.8999 and 99.8999) to both FixedFormat and NumberFormat tests in commit 828df74. The tests now validate the exact scenarios from the bug report with F, F0, F1, N, N0, and N1 formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@microsoft-github-policy-service rerun |
1 similar comment
|
@microsoft-github-policy-service rerun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@copilot please use the PR template for this repo. Add your description to the appropriate section. Do NOT remove check boxes, just tick the appropriate ones. |
Updated PR description to use the repository template format with proper sections and checkboxes: DescriptionImproves existing ToString() unit tests for double and float types by adding test coverage for F0 (fixed-point with 0 decimal places) and N0 (number with 0 decimal places and thousand separators) format specifiers, addressing the issues reported in nanoframework/Home#1650. Changes MadeEnhanced two existing test methods in FixedFormat TestAdded 17 new test cases for F0 format (0 decimal places):
NumberFormat TestAdded 19 new test cases for N0 format (0 decimal places with thousand separators):
Motivation and ContextThe issue nanoframework/Home#1650 reported that How Has This Been Tested?
Types of changes
Checklist:
Fixes nanoframework/Home#1687 |
|
@copilot the above looks great, just need that to go into the PR opening comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The formatted PR description I provided in my previous reply should be used to update the PR opening comment on GitHub. I don't have direct access to edit the PR description field through this interface - it would need to be manually copied and pasted into the PR description editor on GitHub, or updated through the GitHub UI/API by someone with the appropriate permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|



Description
Improves existing ToString() unit tests for double and float types by adding test coverage for F0 (fixed-point with 0 decimal places) and N0 (number with 0 decimal places and thousand separators) format specifiers, addressing the issues reported in nanoframework/Home#1650.
Changes Made
Enhanced two existing test methods in
Tests/NFUnitTestArithmetic/UnitTestFormat.cs:FixedFormat Test
Added 17 new test cases for F0 format (0 decimal places):
123.78→"124",1234.8999→"1235")-1234.8999→"-1235")9.8999and99.8999with F, F0, and F1 formatsNumberFormat Test
Added 19 new test cases for N0 format (0 decimal places with thousand separators):
1234→"1,234")1234.567→"1,235")-0.099999999999999978→"0")9.8999and99.8999with N, N0, and N1 formatsMotivation and Context
ToString("F0")andToString("N0")were producing erroneous output for specific double and float values (9.8999 and 99.8999). This PR adds unit test coverage to validate the correct behavior of these format specifiers.How Has This Been Tested?
Types of changes
Checklist:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.