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

Replaced valuecheckMatrix() with CompareMatrices(). Updated some unit tests use Google Test. #1953

Merged

Conversation

liangfok
Copy link
Contributor

Closes #1916.

Getting this merged is a prerequisite for #1898.

Chien-Liang Fok added 30 commits March 24, 2016 15:14
…must_be_implicitly_convertible_to_U error.
…d logic for handling corner cases like infinity, NaN, and tolerance.
# Conflicts:
#	drake/solvers/test/testOptimizationProblem.cpp
@@ -3,9 +3,34 @@
#include "drake/solvers/Optimization.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

alphabetize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@david-german-tri
Copy link
Contributor

Added a bunch of minor comments. Obviously, many of the tests you've migrated are still in a clunky state, but I didn't pile comments onto those - this change is a huge improvement!

@liangfok
Copy link
Contributor Author

@david-german-tri: Thanks for the thorough code review. I believe I addressed all of your comments. Can you verify the changes and merge if everything is acceptable?

InputOutputRelation::Form::POLYNOMIAL);

EXPECT_EQ(InputOutputRelation::composeWith(f, g).form,
InputOutputRelation::Form::POLYNOMIAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like everything should be clang-formatted again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@david-german-tri: I just ran clang-format and noticed it may be violating the style guide's specification on the order of includes. Here is an example change made by clang-format:

screen shot 2016-03-31 at 3 00 24 pm

Shouldn't gtest.h be considered "other libraries' .h" meaning it should go before drake's header files?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the problem that you should use <gtest/gtest.h> and not "gtest/gtest.h"?

@liangfok
Copy link
Contributor Author

The CI servers found a problem that I failed to detect due to the fact that I did not have Matlab installed on any of my local machines:

/home/ubuntu/workspace/experimental/compiler/gcc/label/linux_matlab/drake/systems/plants/test/compareParsersmex.cpp: In function ‘void mexFunction(int, mxArray**, int, const mxArray**)’:
/home/ubuntu/workspace/experimental/compiler/gcc/label/linux_matlab/drake/systems/plants/test/compareParsersmex.cpp:104:64: error: ‘valuecheckMatrix’ was not declared in this scope
       valuecheckMatrix(matlab_H, cpp_H, 1e-8, "H doesn't match");
                                                                ^
/home/ubuntu/workspace/experimental/compiler/gcc/label/linux_matlab/drake/systems/plants/test/compareParsersmex.cpp:116:55: error: ‘valuecheckMatrix’ was not declared in this scope
                        "joint_limit_min doesn't match");
                                                       ^
/home/ubuntu/workspace/experimental/compiler/gcc/label/linux_matlab/drake/systems/plants/test/compareParsersmex.cpp:125:70: error: ‘valuecheckMatrix’ was not declared in this scope
       valuecheckMatrix(matlab_phi, cpp_phi, 1e-8, "phi doesn't match");
                                                                      ^

@jwnimmer-tri
Copy link
Collaborator

Here's a final suggestion, taken from the changelog -- for any users who consult this PR for advice:

Unit tests that originally contained

    valuecheckMatrix(matrix1, matrix2, tolerance);

should be modified to use Google Test and the following line:

    EXPECT_TRUE(drake::util::CompareMatrices(matrix1, matrix2, tolerance, MatrixCompareType::absolute));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants