Added Distance unit tests #585

kaitlyndlee · 2018-11-16T23:48:39Z

I will be refactoring this test since there are areas where I can use parameters. I also made some comments about some questions I had.

kaitlyndlee · 2018-11-16T23:52:52Z

isis/tests/DistanceTests.cpp

+}
+
+
+TEST(DistanceTests, MetersConstructor) {


I am not sure if using parameters would make things nicer here. My only idea would be to use a Qlist of QLists where each QList element is made up of Distance::<unit_type> and every value the original distance gets converted to in all units. Each test has the conversions in different orders, so each test would need a different ordered list.

kaitlyndlee · 2018-11-16T23:55:18Z

isis/tests/DistanceTests.cpp

+}
+
+
+//should we do all units?


Since we have checked above that we can convert other units to miles, and each "mathy" test converts all units into miles before calculating, is it safe to say that we don't need to test every combination of units in the tests below? I just used meters for everything.

This seems okay to me!

kaitlyndlee · 2018-11-17T00:07:15Z

There are two helper methods, distance() and setDistance(), that are used in the constructor and the getters. I skipped testing these, but there are exceptions that can be thrown if we actually call either one of these methods directly. Is it worth testing these methods?

krlberry · 2018-11-20T02:44:22Z

isis/tests/DistanceTests.cpp

+TEST(DistanceTests, LessThanNull) {
+	try {
+		bool value = Distance() < Distance();
+		ASSERT_TRUE(value);


What is this ASSERT_TRUE for if the comparison in the above line is supposed to throw an exception?

I agree, this should be removed. We shouldn't be testing what that expression outputs because it throws an exception. If we are throwing an exception we should not also be honoring a specific return value.

I forgot to make a comment about that when writing the test. The only reason why I instantiate a bool and use ASSERT_TRUE is so we do not get a build warning saying that it is unused. I would not have put it in otherwise. Would it be better to take it out and have the build warning?

You can just remove the assignment and throw away the result of the comparison.

try { Distance() < Distance(); FAIL() << "Failure Message"; }

That is what I did at first and I get an "unused comparison" warning. We could turn this type of warning off.

Oh! This is fine then.

krlberry

@kdl222 If you want to test the exceptions thrown by setDistance(), you could call the Distance::Distance(double distance, Units distanceUnit) constructor with some kind of invalid units. It looks to me like you are already testing the "negative distance" case. I don't think there's a need to test setDistance() more directly, given how it is used. I don't think you need to test distance() any more unless you'd like to add an exception test, as it is already being tested via other functions.

krlberry · 2018-11-20T03:04:08Z

One more comment -- since the values used are doubles, could you use EXPECT_DOUBLE_EQ, rather than EXPECT_FLOAT_EQ ?

jessemapel

I think better numbers for the test can result in a better test. There is also a weird inconsistency between using EXPECT and ASSERT. You also should be using EXPECT_DOUBLE_EQ instead of EXPECT_FLOAT_EQ.

jessemapel · 2018-11-20T17:30:20Z

isis/tests/DistanceTests.cpp

+	Distance dist(1500500, Distance::Meters);
+	EXPECT_EQ(dist.meters(), 1500500);
+	EXPECT_FLOAT_EQ(dist.kilometers(), 1500.5);
+	EXPECT_FLOAT_EQ(dist.solarRadii(), 0.002155922);


Use EXPECT_DBL_EQ for these comparisons. For the solar radii test, use the actual calculation for the comparison 1500500 / 6.9599e8. Whenever comparing double values, try to avoid comparing against literals that are not 100% exact.

See my other comment and use 1 solar radii = 6.9599e8 meters = 6.9599e5 km = 6.9599e8 pixels (with 1m per pixel) because all of the math will work out exactly.

jessemapel · 2018-11-20T17:30:42Z

isis/tests/DistanceTests.cpp

+	Distance dist(1500.5, Distance::Kilometers);
+	EXPECT_FLOAT_EQ(dist.kilometers(), 1500.5);
+	EXPECT_EQ(dist.meters(), 1500500);
+	EXPECT_FLOAT_EQ(dist.solarRadii(), 0.002155922);


same here, use the exact value to compare against.

See my other comment and use 1 solar radii = 6.9599e8 meters = 6.9599e5 km = 6.9599e8 pixels (with 1m per pixel) because all of the math will work out exactly.

jessemapel · 2018-11-20T17:33:22Z

isis/tests/DistanceTests.cpp

+	EXPECT_FLOAT_EQ(dist.solarRadii(), 0.002155922);
+	EXPECT_FLOAT_EQ(dist.meters(), 1.5005e+06);
+	EXPECT_FLOAT_EQ(dist.kilometers(), 1500.5);
+	EXPECT_FLOAT_EQ(dist.pixels(1), 1.5005e+06);


For this, you should use something simple like 1 solar radii. That will make all of the numbers exact. 1 solar radii = 6.9599e8 meters = 6.9599e5 km = 6.9599e8 pixels (with 1m per pixel).

jessemapel · 2018-11-20T17:38:50Z

isis/tests/DistanceTests.cpp

+	EXPECT_EQ(dist.meters(), 1500500);
+	EXPECT_FLOAT_EQ(dist.kilometers(), 1500.5);
+	EXPECT_FLOAT_EQ(dist.solarRadii(), 0.002155922);
+}


See my other comment and use 1 solar radii = 6.9599e8 meters = 6.9599e5 km = 6.9599e8 pixels (with 1m per pixel) because all of the math will work out exactly.

jessemapel · 2018-11-20T17:41:03Z

isis/tests/DistanceTests.cpp

+	EXPECT_EQ(dist.pixels(2), 1500500);
+	EXPECT_EQ(dist.meters(), 750250);
+	EXPECT_FLOAT_EQ(dist.kilometers(), 750.25);
+	EXPECT_FLOAT_EQ(dist.solarRadii(), 0.0010779609);


See my other comment and use 1 solar radii = 6.9599e8 meters = 6.9599e5 km = 1.39198e9 pixels (with 2m per pixel) because all of the math will work out exactly.

jessemapel · 2018-11-20T17:41:25Z

isis/tests/DistanceTests.cpp

+TEST(DistanceTests, CopyConstructor) {
+	Distance origDist(1500.5, Distance::Meters);
+	Distance copiedDist(origDist);
+	ASSERT_FLOAT_EQ(copiedDist.meters(), 1500.5);


This can be ASSERT_EQ or EXPECT_EQ

jessemapel · 2018-11-20T17:42:59Z

isis/tests/DistanceTests.cpp

+// TEST(DistanceTests, ToString) {
+// 	Distance dist(1500500, Distance::Meters);
+// 	ASSERT_EQ(dist.toString(), "1500500 meters");
+// }


Why is this untested?

jessemapel · 2018-11-20T17:43:48Z

isis/tests/DistanceTests.cpp

+}
+
+
+//should we do all units?


jessemapel · 2018-11-20T17:45:35Z

isis/tests/DistanceTests.cpp

+TEST(DistanceTests, LessThanNull) {
+	try {
+		bool value = Distance() < Distance();
+		ASSERT_TRUE(value);


I agree, this should be removed. We shouldn't be testing what that expression outputs because it throws an exception. If we are throwing an exception we should not also be honoring a specific return value.

…est to use one solar radii.

* Updated the cmake version to 3.10 * Adding configurations for gtest * Tweaking things for gtest * Got gtest working and made a small example test * Updated the cmake version to be 3.10 or greater * Added initial Angle test refactor to use gtest * Add IException and QDebug tests * Added the rest of the IException tests * Added test file discovery support and seperated the main into another file * Added more tests for FileName * Updated with the new testing guidelines and added new tests. * Added Spectel unittest for gtest environment * Added gmock * Forgot to save a file * PixelTest (#536) * initial Pixel test * Add more test cases for Pixel * testing parameterization in Pixel test * testing function parameterization * parameterize static vs object methods * basic PixelTests implementation * clean up and more static tests * update PixelTests with float/dbl expects * Added Endian unit test. * Angle test refactor to use gtest (#529) * Added initial Angle test refactor to use gtest * Add IException and QDebug tests * Added the rest of the IException tests * Update Angle to make private methods public and add tests * Made changes for review comments * Cleanup and addition of streaming the actual errors thrown to the EXPECT_TRUE macro * FileName gtest conversion (#568) * Made two methods private as they are only used within FileName * Added more tests * Added more tests * Update FileNameTests.cpp * BundleSettings unit test (#538) * added build to git ignore * Fixed duplicate main in FileNameTests * Added parameterized test example * Added more tests * Mored BundleSettings tests * Even more tests * Added mocked targetbody settings for BundleSettings test * Added BundleSettings save tests * Fixed BundleSetting save tests to just use a QString * Added default and copy constructor tests and asignment * Update FileName test and merge dev into testing (#598) * Update CMakeLists.txt with version 3.6.0 (patch 1) * Update cmake/CMakeLists.txt library version * Updated the Installation instructions based on user feedback. Also fixed a problem with the isis3VarInit.py script. * Updates made in accordance with requests made on this PR by other developers. * Fix tgo cassis unit test correctly * Fix tgo cassis unit test correctly (#544) * Fixed two bug with AutoReg. One occurs with one dimensional pattern cubes and the other causes the revious registration sample/line to be returned on sub-pixel registration failure. * Fix tgo cassis camera truth again (#554) * Update docsys Makefile to support wwwdoc (#559) The documentation is installed under `docs`, not `doc`, so this file has been updated. `setisis isis3.6.0` and running `make wwwdoc` in `$ISISROOT/../isis/src/docsys` will properly sync the documentation to our web server. * Update isis3VarInit.py Unnecessary 'touch'ing, the '>' redirect will create the file if it doesn't exist. Also, the first redirect should be the single '>'. Otherwise, multiple runs of this program will continue appending to these files. They still work, its just a lot of setting and unsetting for no good reason. * Fixed isis3Startup scripts * Removed unneeded LineEquation include * Make this program more pythonic (#589) * Updated documentation, user interaction, and made the file and directory handling more 'pythonic'. * Correcting for PR comments. * Fixed broken links in the Installation instructions due to the hyperlinks to our GitHub wiki changing. * Removed unused Parabola class * Removed old license, added new unlicense (#594) * Tweaked FileName test to use ISISROOT variable * App to callable function conversion rough draft (#530) * adjusted Process to support lambdas with captures, updated crop app * now compiling with colocated funcs and apps and duplicated symbols accross apps removed * removed debug prints in cmake scripts * changes as requested from review * hapke comment * Added Distance unit tests (#585) * Added Distance unit tests. * Changed EXPECT_FLOAT_EQ to EXPECT_DOUBLE_EQ and changed Solar Radii test to use one solar radii. * Changed unneeded EXPECT_DOUBLE_EQ to EXPECT_EQ. * Fixed toString test. * Adding gtests for Color and Matrix (#532) * Adding gtests for Color and Matrix * Updating gtest for Matrix * More work to ColorTests.cpp and MatrixTests.cpp * Added test for constructor making use of TNT:Array2D in MatrixTests * Update isis/tests/MatrixTests.cpp Co-Authored-By: SgStapleton <[email protected]> * Update isis/tests/MatrixTests.cpp Co-Authored-By: SgStapleton <[email protected]> * Making recommended changes * Fixing odd GitHub behavior... There were suggested changes made in PR that were not playing nicely. Could not push correct version for whatever reason. * Add Utilities, clean up testing (#604) * Update CMakeLists.txt with version 3.6.0 (patch 1) * Update cmake/CMakeLists.txt library version * Updated the Installation instructions based on user feedback. Also fixed a problem with the isis3VarInit.py script. * Updates made in accordance with requests made on this PR by other developers. * Fix tgo cassis unit test correctly * Fix tgo cassis unit test correctly (#544) * Fixed two bug with AutoReg. One occurs with one dimensional pattern cubes and the other causes the revious registration sample/line to be returned on sub-pixel registration failure. * Fix tgo cassis camera truth again (#554) * Update docsys Makefile to support wwwdoc (#559) The documentation is installed under `docs`, not `doc`, so this file has been updated. `setisis isis3.6.0` and running `make wwwdoc` in `$ISISROOT/../isis/src/docsys` will properly sync the documentation to our web server. * Update isis3VarInit.py Unnecessary 'touch'ing, the '>' redirect will create the file if it doesn't exist. Also, the first redirect should be the single '>'. Otherwise, multiple runs of this program will continue appending to these files. They still work, its just a lot of setting and unsetting for no good reason. * Fixed isis3Startup scripts * Removed unneeded LineEquation include * Make this program more pythonic (#589) * Updated documentation, user interaction, and made the file and directory handling more 'pythonic'. * Correcting for PR comments. * Fixed broken links in the Installation instructions due to the hyperlinks to our GitHub wiki changing. * Removed unused Parabola class * Removed old license, added new unlicense (#594) * Fixed issue with the License change (#599) * Removed old license, added new unlicense * Fixed cmake looking for license in the wrong place * Added TestUtilities and used in Color tests * Fixed an issue where app xmls were not being copied * Tweaked IException test failure messages * Updating TestUtilities.h and associated files to account for AssertIException to be broken into AssertIExceptionMessage and AssertIExceptionError

Added Distance unit tests.

638e2fd

kaitlyndlee commented Nov 16, 2018

View reviewed changes

kaitlyndlee requested review from chrisryancombs, jessemapel, krlberry, Kelvinrr and SgStapleton November 16, 2018 23:56

krlberry reviewed Nov 20, 2018

View reviewed changes

krlberry approved these changes Nov 20, 2018

View reviewed changes

jessemapel suggested changes Nov 20, 2018

View reviewed changes

kaitlyndlee added 3 commits November 20, 2018 15:20

Changed EXPECT_FLOAT_EQ to EXPECT_DOUBLE_EQ and changed Solar Radii t…

6c1a4fe

…est to use one solar radii.

Changed unneeded EXPECT_DOUBLE_EQ to EXPECT_EQ.

5fdf565

Fixed toString test.

d5b66b1

jessemapel approved these changes Nov 20, 2018

View reviewed changes

jessemapel merged commit 28338e1 into DOI-USGS:testing Nov 20, 2018

kaitlyndlee deleted the distanceTests branch December 17, 2018 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Distance unit tests #585

Added Distance unit tests #585

kaitlyndlee commented Nov 16, 2018

kaitlyndlee Nov 16, 2018 •

edited

Loading

kaitlyndlee Nov 16, 2018 •

edited

Loading

krlberry Nov 20, 2018

jessemapel Nov 20, 2018

kaitlyndlee commented Nov 17, 2018

krlberry Nov 20, 2018

jessemapel Nov 20, 2018

kaitlyndlee Nov 20, 2018

jessemapel Nov 20, 2018 •

edited

Loading

kaitlyndlee Nov 20, 2018 •

edited

Loading

jessemapel Nov 20, 2018

krlberry left a comment •

edited

Loading

krlberry commented Nov 20, 2018

jessemapel left a comment

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

jessemapel Nov 20, 2018

Added Distance unit tests #585

Added Distance unit tests #585

Conversation

kaitlyndlee commented Nov 16, 2018

kaitlyndlee Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

kaitlyndlee Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaitlyndlee commented Nov 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessemapel Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

kaitlyndlee Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krlberry left a comment • edited Loading

Choose a reason for hiding this comment

krlberry commented Nov 20, 2018

jessemapel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaitlyndlee Nov 16, 2018 •

edited

Loading

kaitlyndlee Nov 16, 2018 •

edited

Loading

jessemapel Nov 20, 2018 •

edited

Loading

kaitlyndlee Nov 20, 2018 •

edited

Loading

krlberry left a comment •

edited

Loading