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.

Sign up for GitHub

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

Added Distance unit tests #585

Merged
merged 4 commits into from
Nov 20, 2018
Merged

Added Distance unit tests #585

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
638e2fd
Added Distance unit tests.
kaitlyndlee Nov 16, 2018
6c1a4fe
Changed EXPECT_FLOAT_EQ to EXPECT_DOUBLE_EQ and changed Solar Radii t…
kaitlyndlee Nov 20, 2018
5fdf565
Changed unneeded EXPECT_DOUBLE_EQ to EXPECT_EQ.
kaitlyndlee Nov 20, 2018
d5b66b1
Fixed toString test.
kaitlyndlee Nov 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
295 changes: 295 additions & 0 deletions isis/tests/DistanceTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,295 @@
#include "Distance.h"
#include "Displacement.h"
#include "IException.h"
#include "SpecialPixel.h"

#include <gtest/gtest.h>

using namespace Isis;

TEST(DistanceTests, DefaultConstructor) {
Distance dist;
EXPECT_EQ(dist.meters(), Null);
EXPECT_EQ(dist.kilometers(), Null);
EXPECT_EQ(dist.pixels(), Null);
EXPECT_EQ(dist.solarRadii(), Null);
}


TEST(DistanceTests, MetersConstructor) {
Copy link
Contributor Author

@kaitlyndlee kaitlyndlee Nov 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

EXPECT_EQ(dist.pixels(1), 1500500);
}


TEST(DistanceTests, KilometersConstructor) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, use the exact value to compare against.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

EXPECT_EQ(dist.pixels(1), 1500500);
}


TEST(DistanceTests, SolarRadiiConstructor) {
Distance dist(0.002155922, Distance::SolarRadii);
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

}


TEST(DistanceTests, PixelsConstructor) {
Distance dist(1500500, Distance::Pixels);
EXPECT_EQ(dist.pixels(1), 1500500);
EXPECT_EQ(dist.meters(), 1500500);
EXPECT_FLOAT_EQ(dist.kilometers(), 1500.5);
EXPECT_FLOAT_EQ(dist.solarRadii(), 0.002155922);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.



TEST(DistanceTests, PixelsPerMeterConstructor) {
Distance dist(1500500, 2);
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}


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

Choose a reason for hiding this comment

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

This can be ASSERT_EQ or EXPECT_EQ

}


TEST(DistanceTests, SetMeters) {
Distance dist;
dist.setMeters(1500500);
ASSERT_EQ(dist.meters(), 1500500);
}


TEST(DistanceTests, SetKilometers) {
Distance dist;
dist.setKilometers(1500500);
ASSERT_EQ(dist.kilometers(), 1500500);
}


TEST(DistanceTests, SetSolarRadii) {
Distance dist;
dist.setSolarRadii(2);
ASSERT_EQ(dist.solarRadii(), 2);
}


TEST(DistanceTests, SetPixels) {
Distance dist;
dist.setPixels(1500500, 2);
ASSERT_EQ(dist.pixels(2), 1500500);
}


TEST(DistanceTests, SetNegativeDistance) {
try {
Distance dist;
dist.setMeters(-1);
FAIL() << "Expected error message: Negative distances are not supported";
}
catch(IException &e) {
EXPECT_TRUE(e.toString().contains("Negative distances are not supported"))
<< e.toString().toStdString();
}
catch(...) {
FAIL() << "Expected error message: Negative distances are not supported";
}
}


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

Choose a reason for hiding this comment

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

Why is this untested?



TEST(DistanceTests, IsValidTrue) {
Distance dist(1500500, Distance::Meters);
ASSERT_TRUE(dist.isValid());
}


TEST(DistanceTests, IsValidFalse) {
Distance dist;
ASSERT_FALSE(dist.isValid());
}


//should we do all units?
Copy link
Contributor Author

@kaitlyndlee kaitlyndlee Nov 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems okay to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

TEST(DistanceTests, GreaterThanDifferent) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
EXPECT_TRUE(dist2 > dist1);
EXPECT_FALSE(dist1 > dist2);
}


TEST(DistanceTests, GreaterThanEqual) {
Distance dist1(10, Distance::Meters);
Distance dist2(10, Distance::Meters);
EXPECT_FALSE(dist2 > dist1);
EXPECT_FALSE(dist1 > dist2);
}


TEST(DistanceTests, GreaterThanNull) {
try {
bool value = Distance() < Distance();
ASSERT_TRUE(value);
FAIL() << "Expected error message: Distance has not been initialized";
}
catch(IException &e) {
EXPECT_TRUE(e.toString().contains("Distance has not been initialized"))
<< e.toString().toStdString();
}
catch(...) {
FAIL() << "Expected error message: Distance has not been initialized";
}
}


TEST(DistanceTests, LessThanDifferent) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
EXPECT_TRUE(dist1 < dist2);
EXPECT_FALSE(dist2 < dist1);
}


TEST(DistanceTests, LessThanEqual) {
Distance dist1(10, Distance::Meters);
Distance dist2(10, Distance::Meters);
EXPECT_FALSE(dist1 < dist2);
EXPECT_FALSE(dist2 < dist1);
}


TEST(DistanceTests, LessThanNull) {
try {
bool value = Distance() < Distance();
ASSERT_TRUE(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@jessemapel jessemapel Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@kaitlyndlee kaitlyndlee Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! This is fine then.

FAIL() << "Expected error message: Distance has not been initialized";
}
catch(IException &e) {
EXPECT_TRUE(e.toString().contains("Distance has not been initialized"))
<< e.toString().toStdString();
}
catch(...) {
FAIL() << "Expected error message: Distance has not been initialized";
}
}


TEST(DistanceTests, AssignDistance) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
dist1 = dist2;
EXPECT_EQ(dist1.meters(), 20);
}


TEST(DistanceTests, Add) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
Distance sum = dist1 + dist2;
EXPECT_EQ(sum.meters(), 30);
}


TEST(DistanceTests, SubtractPositive) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
Displacement difference = dist2 - dist1;
EXPECT_EQ(difference.meters(), 10);
}


TEST(DistanceTests, SubtractNegative) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
Displacement difference = dist1 - dist2;
EXPECT_EQ(difference.meters(), -10);
}


TEST(DistanceTests, DivideDistance) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
EXPECT_EQ(dist2 / dist1, 2);
}


TEST(DistanceTests, DivideDouble) {
Distance dist1(10, Distance::Meters);
Distance quotient = dist1 / 2;
EXPECT_EQ(quotient.meters(), 5);
}


TEST(DistanceTests, Multiply) {
Distance dist1(10, Distance::Meters);
Distance product = dist1 * 2;
EXPECT_EQ(product.meters(), 20);
}


TEST(DistanceTests, AddAssign) {
Distance dist1(10, Distance::Meters);
Distance dist2(20, Distance::Meters);
dist1 += dist2;
EXPECT_EQ(dist1.meters(), 30);
}


TEST(DistanceTests, SubtractAssignPositive) {
Distance dist1(10, Distance::Meters);
Distance dist2(30, Distance::Meters);
dist2 -= dist1;
EXPECT_EQ(dist2.meters(), 20);
}


TEST(DistanceTests, SubtractAssignNegative) {
try {
Distance dist1(10, Distance::Meters);
Distance dist2(30, Distance::Meters);
dist1 -= dist2;
FAIL() << "Expected error message: Negative distances are not supported";
}
catch(IException &e) {
EXPECT_TRUE(e.toString().contains("Negative distances are not supported"))
<< e.toString().toStdString();
}
catch(...) {
FAIL() << "Expected error message: Negative distances are not supported";
}
}


TEST(DistanceTests, DivideAssign) {
Distance dist(10, Distance::Meters);
dist /= 2;
EXPECT_EQ(dist.meters(), 5);
}


TEST(DistanceTests, MultiplyAssign) {
Distance dist(10, Distance::Meters);
dist *= 2;
EXPECT_EQ(dist.meters(), 20);
}