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

NumberToStringGTest: add #include, avoid narrowing conversion pow, mention floating point type on an expect failure #3751

Merged

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Nov 17, 2022

A few small improvements of NumberToStringGTest, triggered by the NumberToString.DecimalNotationUpTo21Digits failures reported by @yurivict at issue #3739 saying:

/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
Expected equality of these values:
  numberToString(power_of_ten)
    Which is: "100000010000"
  '1' + std::string(exponent, '0')
    Which is: "100000000000"

@github-actions github-actions bot added area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Nov 17, 2022
@N-Dekker N-Dekker mentioned this pull request Nov 17, 2022
@N-Dekker
Copy link
Contributor Author

Thanks for your approval @dzenanz. Before merging please let's wait for @yurivict to check if it really fixes the NumberToStringGTest failures at FreeBSD 13.1/clang-14 (issue #3739). (Otherwise this is still a nice STYLE PR, IMhO.)

@yurivict
Copy link

This PR does not resolve the failure.

382/4247 Testing: NumberToString.DecimalNotationUpTo21Digits
382/4247 Test: NumberToString.DecimalNotationUpTo21Digits
Command: "/disk-samsung/freebsd-ports/science/InsightToolkit/work/.build/bin/ITKCommonGTestDriver" "--gtest_filter=NumberToString.DecimalNotationUpTo21Digits" "--gtest_also_run_disabled_tests"
Directory: /disk-samsung/freebsd-ports/science/InsightToolkit/work/.build/Modules/Core/Common/test
"NumberToString.DecimalNotationUpTo21Digits" start time: Nov 19 17:42 PST
Output:
----------------------------------------------------------
Running main() from /wrkdirs/usr/ports/devel/googletest/work/googletest-release-1.12.1/googletest/src/gtest_main.cc
Note: Google Test filter = NumberToString.DecimalNotationUpTo21Digits
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from NumberToString
[ RUN      ] NumberToString.DecimalNotationUpTo21Digits
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.3rc04/Modules/Core/Common/test/itkNumberToStringGTest.cxx:88: Failure
Expected equality of these values:
  numberToString(power_of_ten)
    Which is: "100000010000"
  '1' + std::string(exponent, '0')
    Which is: "100000000000"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.3rc04/Modules/Core/Common/test/itkNumberToStringGTest.cxx:89: Failure
Expected equality of these values:
  numberToString(-power_of_ten)
    Which is: "-100000010000"
  "-1" + std::string(exponent, '0')
    Which is: "-100000000000"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.3rc04/Modules/Core/Common/test/itkNumberToStringGTest.cxx:97: Failure
Expected equality of these values:
  numberToString(power_of_ten)
    Which is: "0.000009999999999999999"
  "0." + std::string(-1 - exponent, '0') + '1'
    Which is: "0.00001"
/disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.3rc04/Modules/Core/Common/test/itkNumberToStringGTest.cxx:98: Failure
Expected equality of these values:
  numberToString(-power_of_ten)
    Which is: "-0.000009999999999999999"
  "-0." + std::string(-1 - exponent, '0') + '1'
    Which is: "-0.00001"
[  FAILED  ] NumberToString.DecimalNotationUpTo21Digits (0 ms)
[----------] 1 test from NumberToString (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] NumberToString.DecimalNotationUpTo21Digits
 
 1 FAILED TEST

Added `#include <cmath>`, to ensure that all `std::pow` overloads are included.

Explicitly declared the local `power_of_ten` variables as `TValue` and
prevented a narrowing conversion from those `std::pow` calls, just to be sure.
Aims to ease the analysis of NumberToStringGTest test failures, like the
`NumberToString.DecimalNotationUpTo21Digits` failures reported by yurivict at
InsightSoftwareConsortium#3739 saying:

    /disk-samsung/freebsd-ports/science/InsightToolkit/work/ITK-5.2.1/Modules/Core/Common/test/itkNumberToStringGTest.cxx:87: Failure
    Expected equality of these values:
      numberToString(power_of_ten)
        Which is: "100000010000"
      '1' + std::string(exponent, '0')
        Which is: "100000000000"
@N-Dekker N-Dekker changed the title BUG: Fix NumberToStringGTest failures by adding missing #include <cmath> NumberToStringGTest: add #include, avoid narrowing conversion pow, mention floating point type on an expect failure Nov 20, 2022
@N-Dekker
Copy link
Contributor Author

Thanks @yurivict I removed the claim that this PR would fix those NumberToString.DecimalNotationUpTo21Digits failures 😞

But I also added an extra commit, with should make the test failures slightly more informative, by mentioning the floating point type ("float" or "double") with each expectation failure. Can you please try that out as well?

@N-Dekker N-Dekker marked this pull request as ready for review November 20, 2022 15:18
@thewtex thewtex added this to the ITK 5.3.1 milestone Nov 23, 2022
@dzenanz dzenanz merged commit 43d3e84 into InsightSoftwareConsortium:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants