tests: fix tests in travis-ci.org #336

commodo · 2017-07-10T14:25:46Z

Seems some failing tests have piled up, due to a missing set -e

test_util_file test ; the issue is that Apple linkers don't allow to override strerror and this seems to be locked down on travis-ci.org [ for security reasons I assume ] ; I tried some solutions from the web (like here: http://tlrobinson.net/blog/2007/12/overriding-library-functions-in-mac-os-x-the-easy-way-dyld_insert_libraries/) ; patches can be checked out here: https://github.com/commodo/json-c/commit/49ed91eb39d96ac625553843ba2c02018f601964 & https://github.com/commodo/json-c/commit/ca13901e0c37a9885fbc665c638185f0ef1c804f ; so, trying to force the test's strerror did not work for me ; they worked locally, but not on travis ; I gave up [ after a few hours ] and resumed to go by checking specific output only for Darwin
test_locale : not sure if the fix is sane ; seems that setlocale() actually changes the locale on Apple, while the uselocale() function does not ;

Signed-off-by: Alexandru Ardelean ardeleanalex@gmail.com

Seems that test1 is failing, but travis is not catching it. Likely, this is because the `cppcheck` returns success and we need to bail on the `make check` step. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

More code compression/de-duplication. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

commodo · 2017-07-11T07:39:04Z

re-spinned to latest master;

hawicz · 2017-07-13T03:56:53Z

json_tokener.c

@@ -239,7 +239,7 @@ struct json_object* json_tokener_parse_ex(struct json_tokener *tok,
 {
  struct json_object *obj = NULL;
  char c = '\1';
-#ifdef HAVE_USELOCALE
+#if defined(HAVE_USELOCALE) && !defined(__APPLE__)


If there's some problem with uselocale or duplocale then the configure test should be adjusted to check for that instead.

hawicz · 2017-07-13T04:02:40Z

tests/test_util_file.expected.Darwin

@@ -0,0 +1,35 @@
+OK: json_object_from_fd(./valid.json)={ "foo": 123 }


Bleh, that sucks about not being able to replace strerror(), but rather than ending up with per-arch output files, let's instead s/strerror/_json_c_strerror/ and then have the tests replace that.

ack;
this will have to be done via a preprocessor #define at build time in .travis.yml ; or maybe via a special configure rule ;
because libjson-c is built first and that would link to strerror

i also thought about renaming strerror
wasn't sure what your thoughts/input would be

ugh... seems cppcheck is not packaged for OS X And `set -e` exposes this. And also `cppcheck` seems to exit with non-zero exit codes by default [even if errs found]. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

Which now seems to fail the build. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

On Apple this seems to fail the `test_locale` test, which would imply that the `uselocale` function does not behave as expected. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

If we want to override `strerror()` in libjson-c to make tests consistent across platforms, we need to do it build-wide as configure/build option. Apple linkers make it really hard to override functions at link-time, and this seems to be locked down on travis-ci.org [ for security reasons I assume ]. While I got it to work locally, it did not work when running on travis. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

To get consistent output between Linux & OS X. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

commodo · 2017-07-13T07:32:40Z

@hawicz
re-spinned
for the strerror override, i chose the way of the "build option"

feel free to disagree with/comment on any of my approaches :)
i'm not completely sure that i chose the best approach[es] ;
well, maybe it's more of a preference thing ;
in which case, i prefer that your preference takes precedence ;

hawicz · 2017-07-14T02:38:31Z

configure.ac

 AC_CHECK_DECLS([INFINITY], [], [], [[#include <math.h>]])
 AC_CHECK_DECLS([nan], [], [], [[#include <math.h>]])
 AC_CHECK_DECLS([isnan], [], [], [[#include <math.h>]])
 AC_CHECK_DECLS([isinf], [], [], [[#include <math.h>]])
 AC_CHECK_DECLS([_isnan], [], [], [[#include <float.h>]])
 AC_CHECK_DECLS([_finite], [], [], [[#include <float.h>]])

+case "${host_os}" in


Why should linux be the only OS allowed to use uselocale? When I said that the configure test needs to be adjusted I mean that it needs to actually check whether uselocale works. This is effectively just an obfuscated #ifdef APPLE.
I'm going to merge this PR since all the other changes look fine, but then I'm going to revert this piece and we'll need to approach it a different way.

hawicz · 2017-07-14T02:51:37Z

ack, wait a sec, this break the tests ALL the time. :(
Having that work conditional on a special config option isn't going to work.

commodo · 2017-07-14T07:31:55Z

when you said about the configure option [instead of the #ifdef APPLE define ] I assumed you have something against #ifdef __PLATOFRM_DEFINES__ ;
the #ifdef APPLE seemed to shortest path to the fix

so, yeah, the uselocale function seems to behave differently on OS X than Linux [ at least in the test_locale test ]
for the setlocale I don't know what's different [if anything] between OS X & Linux

now, i'm also wondering whether it would have worked to fix the test [ add check #ifdef HAVE_USELOCALE #elsif HAVE_SETLOCALE [ same as in in libjson-c] ]
or, maybe that would have just hidden the issue ;

we can insist a bit more on this locale issue
to be honest, i'm just glad tests are back on track ; for me, the locale thing is a bit secondary atm ;
because i would like to get back to JSON patch [ first stop : json_deep_copy() ]
and i'm also thinking about checking into JSON schema later [will see]

there is one element that may crop up later [from the LEDE/OpenWrt/Embedded side of things] ;
some of the stuff that's being added now, grows the lib a bit ;
so, maybe i'll propose some --enable/--disable json-patch/pointer/schema/etc ;
but let's see about this when we get there ; it may not be needed ;

hawicz · 2017-07-15T13:58:30Z

The configure test you added effectively is platform define, rather than a feature check. Whether you use #ifdef APPLE or #ifdef linux or ${host_os} = "linux" you have the same problem: it's only loosely related to the behavior that you're actually trying to adapt to

I'm working on fixing the tests now, since they are definitely not on track with these changes. If we can use the function override functionality, then _json_c_strerror just needs to always be used, with a flag to choose which output you get.

… it on OSX) always include the _json_c_strerror function but only enable it with a flag during tests.

…re using the real strerror.

commodo added 2 commits July 11, 2017 10:38

tests: add set -e specifier to bail early on build run

ba8625a

Seems that test1 is failing, but travis is not catching it. Likely, this is because the `cppcheck` returns success and we need to bail on the `make check` step. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

tests: compress test_utile_file with test_basic

9d47ae8

More code compression/de-duplication. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

hawicz reviewed Jul 13, 2017

View reviewed changes

commodo added 5 commits July 13, 2017 09:05

travis,tests: run cppcheck only if it exists

effab3f

ugh... seems cppcheck is not packaged for OS X And `set -e` exposes this. And also `cppcheck` seems to exit with non-zero exit codes by default [even if errs found]. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

tests: fix leak in test_util_file ; found by cppcheck

7b9432d

Which now seems to fail the build. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

configure.ac: check for uselocale function only on Linux platforms

85f5785

On Apple this seems to fail the `test_locale` test, which would imply that the `uselocale` function does not behave as expected. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

build,travis: enable strerror override option in build

bc2e304

To get consistent output between Linux & OS X. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>

hawicz reviewed Jul 14, 2017

View reviewed changes

hawicz merged commit c0b7d76 into json-c:master Jul 14, 2017

commodo deleted the fix-tests branch July 14, 2017 17:30

hawicz added a commit that referenced this pull request Jul 15, 2017

PR #336: since we can't use function overriding (due to problems with…

730ab7b

… it on OSX) always include the _json_c_strerror function but only enable it with a flag during tests.

hawicz added a commit that referenced this pull request Jul 15, 2017

PR #336: since we can't use function overriding (due to problems with…

fcad0ec

… it on OSX) always include the _json_c_strerror function but only enable it with a flag during tests.

hawicz added a commit that referenced this pull request Jul 28, 2017

PR #336: Fix typo in defining STRERROR_OVERRIDE_IMPL

36dbe2d

hawicz added a commit that referenced this pull request Jul 28, 2017

PR #336: fix to previous change, be sure to include string.h when we'…

af87944

…re using the real strerror.

alexukr mentioned this pull request Aug 9, 2017

MSVC linker error json_c_strerror #347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: fix tests in travis-ci.org #336

tests: fix tests in travis-ci.org #336

commodo commented Jul 10, 2017 •

edited

commodo commented Jul 11, 2017

hawicz Jul 13, 2017

commodo Jul 13, 2017

hawicz Jul 13, 2017

commodo Jul 13, 2017

commodo commented Jul 13, 2017

hawicz Jul 14, 2017

hawicz commented Jul 14, 2017

commodo commented Jul 14, 2017

hawicz commented Jul 15, 2017

		@@ -0,0 +1,35 @@
		OK: json_object_from_fd(./valid.json)={ "foo": 123 }

tests: fix tests in travis-ci.org #336

tests: fix tests in travis-ci.org #336

Conversation

commodo commented Jul 10, 2017 • edited

commodo commented Jul 11, 2017

hawicz Jul 13, 2017

Choose a reason for hiding this comment

commodo Jul 13, 2017

Choose a reason for hiding this comment

hawicz Jul 13, 2017

Choose a reason for hiding this comment

commodo Jul 13, 2017

Choose a reason for hiding this comment

commodo commented Jul 13, 2017

hawicz Jul 14, 2017

Choose a reason for hiding this comment

hawicz commented Jul 14, 2017

commodo commented Jul 14, 2017

hawicz commented Jul 15, 2017

commodo commented Jul 10, 2017 •

edited