Add proj_errno_string() to proj.def #1050

kbevers · 2018-06-20T09:48:39Z

Resolves #1049

rouault · 2018-06-20T10:09:01Z

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 · 2018-06-20T11:21:17Z

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

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.

schwehr · 2018-06-20T16:15:36Z

test/unit/CMakeLists.txt

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


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

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

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)

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.

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.

schwehr · 2018-06-20T16:17:00Z

test/unit/proj_errno_string_test.cpp

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


Please change the author.

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

schwehr · 2018-06-20T16:18:20Z

test/unit/proj_errno_string_test.cpp

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


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.

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.

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.

schwehr · 2018-06-20T16:19:27Z

test/unit/proj_errno_string_test.cpp

+    ASSERT_STREQ("invalid projection system error (-1000)", proj_errno_string(-1000));
+
+    // Test POSIX error numbers
+    #ifdef HAVE_STRERROR


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.

schwehr · 2018-06-20T16:44:49Z

test/unit/proj_errno_string_test.cpp

+
+    // 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 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")); }

schwehr · 2018-06-20T16:46:27Z

test/unit/proj_errno_string_test.cpp

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


Whoops... -9999 != -99999

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

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

schwehr · 2018-06-20T16:49:30Z

test/unit/proj_errno_string_test.cpp

+    // Test the no error result
+    ASSERT_EQ(0, proj_errno_string(0));
+
+    // Test both valid and invalid PROJ error numbers


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

TEST(ProjErrnoStringTest, NegativeErrorNumbers) {

Okay, that makes sense. Thanks.

schwehr · 2018-06-20T17:45:11Z

test/unit/proj_errno_string_test.cpp

+ * DEALINGS IN THE SOFTWARE.
+ ****************************************************************************/
+
+#ifdef HAVE_STRERROR


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

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.

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

I'll just remove it

schwehr · 2018-06-20T17:48:53Z

test/unit/proj_errno_string_test.cpp

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


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.

schwehr · 2018-06-20T17:49:39Z

test/unit/proj_errno_string_test.cpp

+    EXPECT_STREQ("invalid projection system error (-1000)", proj_errno_string(-1000));
+}
+
+TEST(ProjErrnoStringTest, SystemErrnos) {


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

schwehr · 2018-06-20T17:50:31Z

test/unit/proj_errno_string_test.cpp

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


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

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.

schwehr · 2018-06-20T17:52:54Z

test/unit/proj_errno_string_test.cpp

+ * DEALINGS IN THE SOFTWARE.
+ ****************************************************************************/
+
+#ifdef HAVE_STRERROR


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

schwehr · 2018-06-20T18:14:37Z

test/unit/proj_errno_string_test.cpp

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


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

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

schwehr · 2018-06-20T18:14:48Z

test/unit/proj_errno_string_test.cpp

+}
+
+TEST(ProjErrnoStringTest, SystemErrnos) {
+    int max = std::numeric_limits<int>::max();


constexpr int

schwehr · 2018-06-20T18:15:44Z

test/unit/proj_errno_string_test.cpp

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


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

mloskot · 2018-06-20T22:59:06Z

Why basic_test.cpp is needed at all?

schwehr · 2018-06-20T23:04:12Z

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 · 2018-06-21T19:29:51Z

Looks good to me

kbevers · 2018-06-21T19:51:40Z

Thanks for your reviews everyone. They was most helpful!

Add proj_errno_string() to proj.def

d742d57

kbevers added this to the 5.2.0 milestone Jun 20, 2018

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

schwehr requested changes Jun 20, 2018

View reviewed changes

kbevers force-pushed the add-proj_errno_string-proj.def branch from 07c5511 to a50f5b6 Compare June 20, 2018 17:42

schwehr reviewed Jun 20, 2018

View reviewed changes

kbevers force-pushed the add-proj_errno_string-proj.def branch from a50f5b6 to afa3ea8 Compare June 20, 2018 17:46

schwehr reviewed Jun 20, 2018

View reviewed changes

kbevers force-pushed the add-proj_errno_string-proj.def branch from afa3ea8 to 9d390cb Compare June 20, 2018 18:10

schwehr reviewed Jun 20, 2018

View reviewed changes

Add test for proj_errno_string()

b87b591

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

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

kbevers deleted the add-proj_errno_string-proj.def branch July 9, 2018 08:46

schwehr mentioned this pull request Jul 26, 2018

pj_strerrno(): Change check of err value to avoid undefined behavior. #1083

Merged

Add proj_errno_string() to proj.def #1050

Add proj_errno_string() to proj.def #1050

Conversation

kbevers commented Jun 20, 2018

rouault commented Jun 20, 2018

kbevers commented Jun 20, 2018

schwehr 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

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

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

mloskot commented Jun 20, 2018

schwehr commented Jun 20, 2018

rouault commented Jun 21, 2018

kbevers commented Jun 21, 2018