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

tests_printf_float: fixed precision in test case #5383

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

A-Paul
Copy link
Member

@A-Paul A-Paul commented Apr 21, 2016

While testing for Release 2016.04 it turned out that the test always failed. A precision value according to the reference string was missing.

@A-Paul A-Paul added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: tests Area: tests and testing framework labels Apr 21, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Apr 21, 2016

Looks valid but I don't see why adding only one decimal... (or two or three...) This is only for the test?

@kYc0o kYc0o added this to the Release 2016.04 milestone Apr 21, 2016
@kYc0o kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 21, 2016
@kaspar030
Copy link
Contributor

I don't see why adding only one decimal...

Same here, why not check for the full value?

@aabadie
Copy link
Contributor

aabadie commented Apr 21, 2016

Same here, why not check for the full value?

The test was designed like this at the beginning, I'm surprised that with the wrong format string. Checking the full value, or 1 or 2 digits is a mater of taste.
Maybe the test can be improved by testing with all possible digits formatting (1, 2, 3 with valid results and maybe unexpected values like 4) but IMHO that will be a bit overkill.

@haukepetersen
Copy link
Contributor

already proposed a more complete version of the test: #5384

@kYc0o kYc0o modified the milestones: Release 2016.07, Release 2016.04 Apr 25, 2016
@A-Paul
Copy link
Member Author

A-Paul commented Jun 9, 2016

@aabadie the ocurrence of a duplicate PR indicates that there is need to fix this bug.
Can you agree to merge this quick and then wait for improvements like #5384?

@miri64
Copy link
Member

miri64 commented Jun 9, 2016

@aabadie also see #5512 (comment)

@aabadie
Copy link
Contributor

aabadie commented Jun 16, 2016

@A-Paul, @miri64. I think we should merge this one as it's a simple fix for the initial problem. #5384 is a more complete refactoring of the initial test.

ACK

@miri64 miri64 merged commit 1701b12 into RIOT-OS:master Jun 16, 2016
@A-Paul A-Paul deleted the fix_tests_printf_float branch June 16, 2016 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants