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

Projects
None yet
4 participants
@kbevers
Member

kbevers commented Jun 20, 2018

Resolves #1049

@kbevers kbevers added this to the 5.2.0 milestone Jun 20, 2018

@rouault

This comment has been minimized.

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

This comment has been minimized.

Member

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 kbevers:add-proj_errno_string-proj.def branch from a8decd6 to 07c5511 Jun 20, 2018

@schwehr

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)

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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

This comment has been minimized.

@rouault

rouault Jun 20, 2018

Member

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)

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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.

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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>

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

Please change the author.

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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.

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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.

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

Whoops... -9999 != -99999

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

TEST(ProjErrnoStringTest, NegativeErrorNumbers) {

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

Okay, that makes sense. Thanks.

@kbevers kbevers force-pushed the kbevers:add-proj_errno_string-proj.def branch from 07c5511 to a50f5b6 Jun 20, 2018

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

#ifdef HAVE_STRERROR

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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.

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

I'll just remove it

@kbevers kbevers force-pushed the kbevers:add-proj_errno_string-proj.def branch from a50f5b6 to afa3ea8 Jun 20, 2018

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@kbevers

kbevers Jun 20, 2018

Member

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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 kbevers:add-proj_errno_string-proj.def branch from afa3ea8 to 9d390cb Jun 20, 2018

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();

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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();

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

This comment has been minimized.

@schwehr

schwehr Jun 20, 2018

Contributor

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

@kbevers kbevers force-pushed the kbevers:add-proj_errno_string-proj.def branch from 9d390cb to b87b591 Jun 20, 2018

@mloskot

This comment has been minimized.

Member

mloskot commented Jun 20, 2018

Why basic_test.cpp is needed at all?

@schwehr

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

rouault commented Jun 21, 2018

Looks good to me

@kbevers kbevers merged commit 8ee389a into OSGeo:master Jun 21, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 77.662%
Details
@kbevers

This comment has been minimized.

Member

kbevers commented Jun 21, 2018

Thanks for your reviews everyone. They was most helpful!

@kbevers kbevers deleted the kbevers:add-proj_errno_string-proj.def branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment