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

Add proj_errno_string() to proj.def #1050

Merged
merged 2 commits into from Jun 21, 2018

Conversation

kbevers
Copy link
Member

@kbevers kbevers commented Jun 20, 2018

Resolves #1049

@kbevers kbevers added this to the 5.2.0 milestone Jun 20, 2018
@rouault
Copy link
Member

rouault commented Jun 20, 2018

May I suggest to add a call to the function in https://github.com/OSGeo/proj.4/blob/master/test/unit/basic_test.cpp ?

@kbevers
Copy link
Member Author

kbevers commented Jun 20, 2018

Yes you may. I'll see if I can find the time to add those later today.

@kbevers kbevers force-pushed the add-proj_errno_string-proj.def branch from a8decd6 to 07c5511 Compare June 20, 2018 13:48
Copy link
Member

@schwehr schwehr left a comment

Choose a reason for hiding this comment

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

My initial pj_phi2_test.cpp wasn't particularly good. Sorry for a crap example. It needs major help. Most of these comments apply to that test file too.

@@ -52,7 +52,8 @@ unset(_save_cxx_flags)
#
add_executable(proj_test_unit
main.cpp
basic_test.cpp)
basic_test.cpp
proj_errno_string_test.cpp)
Copy link
Member

Choose a reason for hiding this comment

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

I strongly encourage you to not combine compilation units into the same test target. You now must make absolutely sure that proj_errno_string_test.cpp leaves any global in a state that exactly matches as if the tests were never run. If you fail to cleanup or have a test barf, you risk making the problem much harder to follow. Also, if you separate them out, you can run them in parallel in separate processes... I need to figure out how to parallelize that. I'm guess that this doesn't parallelize the unit tests :(

rm -rf build-ninja; (mkdir -p build-ninja && cd build-ninja && cmake -GNinja .. && cmake --build . && ctest .)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I didn't realise what I was doing here. I'll change this to a separate test target.

Copy link
Member

Choose a reason for hiding this comment

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

There will be a significant cost in term of linking time if we add a binary for each test we write. For my C++ SRS stuff, I intend to have just one test binary. Normally PROJ shouldn't cause global side effects, except perhaps the global default context (but in that case we should have a way to reset it between runs)

Copy link
Member

Choose a reason for hiding this comment

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

I've not done testing at scale with cmake or autoconf much, so I differ to @rouault . Go with one test bin and if the world gets too painful, we can pull out a key test file or two to separate binaries. I just want to take advantage of the large number of cores that workstations are starting to have (I've got 28 physical cores for 56 hyperthreads), but I know that the web based CI systems will go really slow if link time is large.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am indifferent at the moment - I'll just go with whatever you guys decide is best. I might come back with a stronger opinion after having played around with the test framework for a while.

*
* Project: PROJ
* Purpose: Unit test for proj_errno_string()
* Author: Kurt Schwehr <schwehr@google.com>
Copy link
Member

Choose a reason for hiding this comment

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

Please change the author.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't want credit for my crappy test? :)

ASSERT_EQ(0, proj_errno_string(0));

// Test both valid and invalid PROJ error numbers
ASSERT_STREQ("no arguments in initialization list", proj_errno_string(-1));
Copy link
Member

Choose a reason for hiding this comment

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

ASSERT_STREQ -> EXPECT_EQ. Try to reserve asserts for things were it is totally unsafe to proceed. By using expect, you get to keep generating test output and fail later. That often makes debugging easier when I do something dumb to the code under test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just did a search for how to test if two strings are equal and this came up. I'll change the ASSERTs to EXPECTs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, _STREQ is the way for strings, but exact full matches on strings are brittle. What if later you rephrase the message or change parens to square brackets, yada yada. I recommend trying to find the most stable parts and probe those.

ASSERT_STREQ("invalid projection system error (-1000)", proj_errno_string(-1000));

// Test POSIX error numbers
#ifdef HAVE_STRERROR
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to always have # start at column 0... it's not C++ code. No indent is a big warning to my brain to beware the C preprocessor.


// Test both valid and invalid PROJ error numbers
ASSERT_STREQ("no arguments in initialization list", proj_errno_string(-1));
ASSERT_STREQ("invalid projection system error (-9999)", proj_errno_string(-9999));
Copy link
Member

Choose a reason for hiding this comment

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

This is the kind of place where a gmock matcher is useful. Match part of the message and in a separate expect, match the number. You can then even make a range based for loop with a std::initializer_list and say something like this that should be less brittle.

#include <limits>

// Warning: untested code...
for (const int val : {-1, -1000, -9999, std::numeric_limits<int>::min()}) {
  // Messages are of the form:
  //     invalid projection system error (-1000)
  const char *msg = proj_errno_string(val);
  EXPECT_THAT(msg, testing::ContainsRegex("invalid projection"));
  // Do this part better.  I can't just use my normal absl.
  char buf[100];
  sprintf(buf, "(%d)", val);
  EXPECT_THAT(msg, testing::ContainsRegex("invalid projection"));
}

// Test both valid and invalid PROJ error numbers
ASSERT_STREQ("no arguments in initialization list", proj_errno_string(-1));
ASSERT_STREQ("invalid projection system error (-9999)", proj_errno_string(-9999));
ASSERT_STREQ("invalid projection system error (-9999)", proj_errno_string(-99999));
Copy link
Member

Choose a reason for hiding this comment

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

Whoops... -9999 != -99999

Copy link
Member Author

Choose a reason for hiding this comment

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

This is on purpose. The function returns -9999 on errnos < -9999. I can change it to another number that is less confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Then a comment so the reader knows that without having to hunt for it.

// Test the no error result
ASSERT_EQ(0, proj_errno_string(0));

// Test both valid and invalid PROJ error numbers
Copy link
Member

Choose a reason for hiding this comment

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

More small tests with good names means that we don't need comments...

TEST(ProjErrnoStringTest, NegativeErrorNumbers) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that makes sense. Thanks.

@kbevers kbevers force-pushed the add-proj_errno_string-proj.def branch from 07c5511 to a50f5b6 Compare June 20, 2018 17:42
* DEALINGS IN THE SOFTWARE.
****************************************************************************/

#ifdef HAVE_STRERROR
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should leave off the #ifdef for system headers like this. It's like pulled in elsewhere anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was mostly meant as a communicate device to say that cstring is only needed in certain cases. I am sure it is included elsewhere. I can remove it if it is just a distraction.

Copy link
Member

Choose a reason for hiding this comment

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

I think it might be a distraction, but maybe @rouault or @mloskot could offer an opinion on if it's helpful or just noise. Or just go with what you think is best. It's not really that important :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just remove it

@kbevers kbevers force-pushed the add-proj_errno_string-proj.def branch from a50f5b6 to afa3ea8 Compare June 20, 2018 17:46
EXPECT_STREQ("no arguments in initialization list", proj_errno_string(-1));
EXPECT_STREQ("invalid projection system error (-9999)", proj_errno_string(-9999));
// for errnos < -9999, -9999 is always returned
EXPECT_STREQ("invalid projection system error (-9999)", proj_errno_string(-50000));
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using -10000 and std::numeric_limits<int>::min() to cover the range of possible values. Using one past makes sure the transition happens where you think it does.
And somethings bad things happen at the extremes.

EXPECT_STREQ("invalid projection system error (-1000)", proj_errno_string(-1000));
}

TEST(ProjErrnoStringTest, SystemErrnos) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a maximum positive value that can be used? What if I call it with std::numeric_limits<int>::max() ?

TEST(ProjErrnoStringTest, SystemErrnos) {
#ifdef HAVE_STRERROR
EXPECT_STREQ(strerror(5), proj_errno_string(5));
EXPECT_STREQ(strerror(9999), proj_errno_string(9999));
Copy link
Member

Choose a reason for hiding this comment

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

So the behavior over 9999 is different between having HAVE_STRERROR and not?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume so, yes. Haven't checked the docs for strerror() but proj_errno_string()[0] returns the error message verbatim from strerror.

[0] really it is pj_strerrno() that does it. proj_errno_string is just a wrapper.

* DEALINGS IN THE SOFTWARE.
****************************************************************************/

#ifdef HAVE_STRERROR
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be a distraction, but maybe @rouault or @mloskot could offer an opinion on if it's helpful or just noise. Or just go with what you think is best. It's not really that important :)

@kbevers kbevers force-pushed the add-proj_errno_string-proj.def branch from afa3ea8 to 9d390cb Compare June 20, 2018 18:10
EXPECT_STREQ("invalid projection system error (-1000)", proj_errno_string(-1000));
EXPECT_STREQ("invalid projection system error (-9999)", proj_errno_string(-9999));
// for errnos < -9999, -9999 is always returned
int min = std::numeric_limits<int>::min();
Copy link
Member

Choose a reason for hiding this comment

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

constexpr isn't super important here, but it's a good habit.

constexpr int min = std::numeric_limits<int>::min();

}

TEST(ProjErrnoStringTest, SystemErrnos) {
int max = std::numeric_limits<int>::max();
Copy link
Member

Choose a reason for hiding this comment

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

constexpr int

EXPECT_STREQ("no system list, errno: 5\n", proj_errno_string(5));
EXPECT_STREQ("no system list, errno: 9999\n", proj_errno_string(9999));
// for errnos > 9999, 9999 is always returned
EXPECT_STREQ("no system list, errno: 9999\n", proj_errno_string(max));
Copy link
Member

Choose a reason for hiding this comment

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

Switch order for maximum clarity... 9999, 10000, max.

@kbevers kbevers force-pushed the add-proj_errno_string-proj.def branch from 9d390cb to b87b591 Compare June 20, 2018 18:24
@mloskot
Copy link
Member

mloskot commented Jun 20, 2018

Why basic_test.cpp is needed at all?

@schwehr
Copy link
Member

schwehr commented Jun 20, 2018

I would argue that basic_test.cpp isn't necessary once we have a range of tests in place, but no need to rush into removing it.

@rouault
Copy link
Member

rouault commented Jun 21, 2018

Looks good to me

@kbevers kbevers merged commit 8ee389a into OSGeo:master Jun 21, 2018
@kbevers
Copy link
Member Author

kbevers commented Jun 21, 2018

Thanks for your reviews everyone. They was most helpful!

@kbevers kbevers deleted the add-proj_errno_string-proj.def branch July 9, 2018 08:46
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.

None yet

4 participants