[PATCH] ldb: Use deterministic order of dict for tests

[Douglas Bagnall]

hi Lukas

On 19/01/19 8:48 AM, Lukas Slebodnik via samba-technical wrote:
> ehlo,
> 
> Unit test failed for me in fedora on s390x
> https://kojipkgs.fedoraproject.org//work/tasks/6615/32106615/build.log
> https://kojipkgs.fedoraproject.org//work/tasks/7969/32107969/build.log
> 
> But there is also a reasonable reproducer on x86_64
> 
> sh$ cat > reproducer.py <<EOF
> import ldb
> msg = ldb.Message()
> msg.dn = ldb.Dn(ldb.Ldb(), "dc=foo29")
> msg["dc"] = b"foo"
> print(repr(msg))
> EOF
> 
> sh$ PYTHONHASHSEED=2 python2 reproducer.py
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> 
> sh$ PYTHONHASHSEED=1 python2 reproducer.py
> Message({'dn': Dn('dc=foo29'), 'dc': MessageElement(['foo'])})
> 
> sh$  for i in {1..10}; do PYTHONHASHSEED=random python2
> reproducer.py; done
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dn': Dn('dc=foo29'), 'dc': MessageElement(['foo'])})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> Message({'dn': Dn('dc=foo29'), 'dc': MessageElement(['foo'])})
> Message({'dc': MessageElement(['foo']), 'dn': Dn('dc=foo29')})
> 
> I was thinking about fixing it in pyldb.c but IMHO it does not worth
> to spend time with fixing it in C in case of trivial workaround when
> executing tests.

yep. However in lib/ldb/Makefile (and other Makefiles) we have:

WAF=PYTHONHASHSEED=1 WAF_MAKE=1 $(WAF_BINARY)

[...]

test:
        $(WAF) test $(TEST_OPTIONS)


Using, as you can see, "1" for the seed (it is there in the s390x
build logs). Evidently that doesn't fall through to the underlying
tests, at least not in all cases. But maybe sometimes it does.

Which leads me to the two things I don't like so much about this patch:

> --- a/lib/ldb/wscript
> +++ b/lib/ldb/wscript
> @@ -559,7 +559,7 @@ def test(ctx):
>      if env.HAVE_LMDB:
>          pyret = samba_utils.RUN_PYTHON_TESTS(
>              ['tests/python/api.py', 'tests/python/index.py'],
> -            extra_env={'SELFTEST_PREFIX': test_prefix})
> +            extra_env={'SELFTEST_PREFIX': test_prefix, 'PYTHONHASHSEED': '0'})
>      else:
>          pyret = 0
>      print("Python testsuite returned %d" % pyret)

1. It would be better to use "'PYTHONHASHSEED': '1'", for consistency.

2. It would be better to have a more general solution that fixed it for
ever for everyone, not just the lmdb ldb tests. Like maybe here:

../buildtools/wafsamba/samba_utils.py:395:def LOAD_ENVIRONMENT()


I don't know. There *might* be someone on the list who understands waf.

cheers,
Douglas