attachment
0001-Ticket-48832-CI-test-fix-ticket-failures.patch

If we are talking about numerous issues with tmp(and data) dir permissions, then I propose to do not change the tests, but change the CI script.

As an option, we can change owner to dirsrv and set selinux contexts properly (thanks Viktor for this script):

{{{
mkdir /export
cd /export
cp -r ds/dirsrvtests/tests /export
chown dirsrv: -R /export
chcon -R -t dirsrv_tmp_t /export
cd /export/tests/
}}}

It is not the only way, of course. Let's discuss. :)

Thanks for fixing them, Simon!

Replying to [comment:2 spichugi]:

If we are talking about numerous issues with tmp(and data) dir permissions, then I propose to do not change the tests, but change the CI script.

As an option, we can change owner to dirsrv and set selinux contexts properly (thanks Viktor for this script):

{{{
mkdir /export
cd /export
cp -r ds/dirsrvtests/tests /export
chown dirsrv: -R /export
chcon -R -t dirsrv_tmp_t /export
cd /export/tests/
}}}

It is not the only way, of course. Let's discuss. :)

As for Viktor's script, how do we automate this? Do we just run this inside every CI test script that uses the TMP/DATA dirs? I think this also assumes we are running the tests as root.

I wonder if it would be best to simply remove the TMP dir stuff, but keep the DATA dir. Then update the README to say that the contents of a DATA dir should be copied to a location that the DS user can access. I do not think DATA dir is widely used anyway, so it should be easy to change it.

Replying to [comment:4 mreynolds]:

Replying to [comment:2 spichugi]:

If we are talking about numerous issues with tmp(and data) dir permissions, then I propose to do not change the tests, but change the CI script.

As an option, we can change owner to dirsrv and set selinux contexts properly (thanks Viktor for this script):

{{{
mkdir /export
cd /export
cp -r ds/dirsrvtests/tests /export
chown dirsrv: -R /export
chcon -R -t dirsrv_tmp_t /export
cd /export/tests/
}}}

It is not the only way, of course. Let's discuss. :)

As for Viktor's script, how do we automate this? Do we just run this inside every CI test script that uses the TMP/DATA dirs? I think this also assumes we are running the tests as root.
We can put it to CI test environment setup. And we can use sudo for running the commands.
[[BR]]

I wonder if it would be best to simply remove the TMP dir stuff, but keep the DATA dir. Then update the README to say that the contents of a DATA dir should be copied to a location that the DS user can access. I do not think DATA dir is widely used anyway, so it should be easy to change it.
I like the idea. We can store all temporary data by using python features, not saving to files. And if we still need some place to save, then we can use /tmp.

As for DATA dir, we can use script above. What do you think?

Since the discussion is going on, I'm undo'ing my ack...

Replying to [comment:6 nhosoi]:

Since the discussion is going on, I'm undo'ing my ack...

Hi Noriko.
Thank you for the review. :)

Discussion is about tmp/data dir.
Patch has fixes that are not related to this topic. So everything is alright, you can pass the ack back. :)

Ok, Simon. Your patch [1] has my ack anyway... If the discussion is outside of the patch, please feel free to push it. Thanks!

[1] 0001-Ticket-48832-CI-test-fix-ticket-failures.patch​

To ssh://git.fedorahosted.org/git/389/ds.git

78f730a..735ccc6 master -> master
commit 735ccc6
Author: Simon Pichugin spichugi@redhat.com
Date: Tue May 24 11:03:55 2016 +0200

There still needs to be jenkins testing done, so there could be another patch needed.

Thank you, Mark!
Patch is really big! While I am going with my eyes through the code, I've run the whole pytest execution for suites and tickets. It will take a while.

I'll come back with the results tomorrow when the run will be done.

Looks good to me.

A very minor thing, but I guess you were troubleshooting this test case 48226?
fin in ticket48226_test.py comments out these delete lines... ;)
{{{
123 123 def fin():
124 master1.delete()
125 master2.delete()
124 #master1.delete()
125 #master2.delete()
}}}

And I'm leaving to set "ack" to Simon...

ticket47536_test.py

{{{
installation1_prefix = ''
}}}

Remove the installation prefix, and use the environment variable. It "just works".

ticket48194_test.py

{{{
LDAPSPORT = '636'
}}}

Can we avoid the actual system ldaps port. IIRC there will be something in properties / constants for "that instance" to use, rather than this.

More review on the way for the second patch ....

reliab_7_5_test.py

{{{
tmp_dir = '/tmp'
}}}

Please use environment tmp instead, don't hardcode this.

Otherwise, it's looking awesome! Thanks for the hardwork on this!

After 4 hours run I have:
============= 36 failed, 522 passed, 10 error in 16149.46 seconds ==============

I've sent an email to you with full output (pretty big file) and credentials of my machine with fully installed testing environment.

Please, skip basic_test.py::test_basic_ldapagent failure. It was okay after an installing of 389-ds-base-snmp.

If you want, I'll go through the failures more closely from the down of the list.

Replying to [comment:16 spichugi]:

After 4 hours run I have:
============= 36 failed, 522 passed, 10 error in 16149.46 seconds ==============

I've sent an email to you with full output (pretty big file) and credentials of my machine with fully installed testing environment.

Please, skip basic_test.py::test_basic_ldapagent failure. It was okay after an installing of 389-ds-base-snmp.

If you want, I'll go through the failures more closely from the down of the list.

A few things:

[1] Make sure you refresh your lib389 source code! There were several key changes that the CI tests will need.
[2] There are still a few test cases that fail due to SELinux, but I'm in the process of fixing those as well. For now I guess you could disable it, but I plan on getting them all fixed today.

New patch attached

All of those changes look pretty simple. I'm happy with it Mark, great work on all of this!

Replying to [comment:19 firstyear]:

All of those changes look pretty simple. I'm happy with it Mark, great work on all of this!

Whoops, this patch is missing a whole commit. New, much larger, patch coming...

Local testing patch (no jenkins)
0001-Ticket-48832-Fix-lib389-CI-ticket-suite-test-failure.patch

New patch attached

Second review looks good, I plan to run this in the next few days to be sure. I'm happy to ack and we can fix the remaining from there.

To ssh://git.fedorahosted.org/git/389/ds.git
e3a7705..1972195 master -> master
commit 1972195
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Jul 20 20:43:19 2016 -0400

Next these tests need to go through a jenkins run before closing this out.

convert test to use py.test
0001-Ticket-48832-CI-tests-convert-all-the-tests-to-use-p.patch

git patch file (master) -- CI test; fixing ticket47838_test.py
0001-Ticket-48832-Fix-lib389-CI-tests.patch

Hi Mark,

The attached patch fixes the test failure for me.
Could you please try the patch?
If it works, could you push it together with your fix?
Thanks!
--noriko

Thanks Noriko, I only needed to add part of your patch. We still have issues with that testcase, but that can be addressed next.

a06cb42..7a92bcf master -> master
commit 7a92bcf
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Jul 26 21:22:26 2016 -0400

More porting issues
0001-Ticket-48832-CI-Tests-make-tests-more-portable.patch

Some tests like this one - '''dirsrvtests/tests/tickets/ticket47838_test.py''' can't be run with py.test command, because it can't recognize tests there (functions have names like _47838_run_10). But it is related to previous patch that was already merged.

Do you want me to do through the tests and fix it? Or have you already started some another big patch (so it will be less 'rebase' problems for both of us, if you will continue this task too)?

But regarding this new patch, everything looks good to me! Ack.

Replying to [comment:29 spichugi]:

Some tests like this one - '''dirsrvtests/tests/tickets/ticket47838_test.py''' can't be run with py.test command, because it can't recognize tests there (functions have names like _47838_run_10). But it is related to previous patch that was already merged.

Do you want me to do through the tests and fix it?

That would be great (if you can getit done today) If not I'll do it

Or have you already started some another big patch (so it will be less 'rebase' problems for both of us, if you will continue this task too)?

No I'm caught up now.

But regarding this new patch, everything looks good to me! Ack.

Thanks

a20538f..724adba master -> master
commit 724adba
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Jul 28 10:52:58 2016 -0400

attachment
0001-Ticket-48832-Fix-pytest-compatibility-in-CI-tests.patch

I've checked all tickets and suites for the pytest cases discovery, and I think we are good here.

Today I will run whole pytest execution for a night to check what's left to fix.

To ssh://git.fedorahosted.org/git/389/ds.git

Pushed to master:
724adba..d193997 master -> master
commit d193997
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Jul 28 20:34:02 2016 +0200

I have a great news! We have nearly done.
After the run I had only 3 failed test suites (on a slow beaker machine).
I already have a fix for two of them, please, take a look at the commit message.

Regarding the last one, I'll ask you to take a look at it. It involves some work with ciphers that you've already worked on. I am sending a email with logs attached now.

Hmmm, I had to remove "localhost.localdomain" and change it to 'localhost" to get these tests to pass on my jenkins server. Here is my hosts file from a F23 machine

{{{

cat /etc/hosts

127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
}}}

So your change would break things in my setup. I guess we need to find a better way to make this more portable. Does your hosts file start with "localhost.localdomain localhost"? Perhaps we should grab the the "local hostname" from /etc/hosts instead of hardcoding it?

For the cipher test issues, what are you OS/version running this on? Note - these are upstream tests so they should work F23 and up(anything else isn't really valid)

Replying to [comment:36 mreynolds]:

Hmmm, I had to remove "localhost.localdomain" and change it to 'localhost" to get these tests to pass on my jenkins server. Here is my hosts file from a F23 machine

{{{

cat /etc/hosts

127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
}}}

So your change would break things in my setup. I guess we need to find a better way to make this more portable. Does your hosts file start with "localhost.localdomain localhost"? Perhaps we should grab the the "local hostname" from /etc/hosts instead of hardcoding it?
Yeah, fair point. What do you think, if I'll add a function like this to lib389?
{{{
def getlocalhost():
with open('/etc/hosts', 'r') as f:
for line in f.readlines():
if line.startswith('127.0.0.1'):
localhost = line.split()[1]
return localhost
return None
}}}
'''/etc/hosts''' has '-rw-r--r--' so everyone can read it.

For the cipher test issues, what are you OS/version running this on? Note - these are upstream tests so they should work F23 and up(anything else isn't really valid)

{{{
[spichugi@auto ~]$ cat /etc/redhat-release
Fedora release 24 (Twenty Four)
}}}
Also, we plan to run tests on downstream too. Will this part work there fine? Or should we skip some tests for RHEL 6 or RHEL 7, 71, 72?

Replying to [comment:37 spichugi]:

So your change would break things in my setup. I guess we need to find a better way to make this more portable. Does your hosts file start with "localhost.localdomain localhost"? Perhaps we should grab the the "local hostname" from /etc/hosts instead of hardcoding it?
Yeah, fair point. What do you think, if I'll add a function like this to lib389?
{{{
def getlocalhost():
with open('/etc/hosts', 'r') as f:
for line in f.readlines():
if line.startswith('127.0.0.1'):
localhost = line.split()[1]
return localhost
return None
}}}
'''/etc/hosts''' has '-rw-r--r--' so everyone can read it.

This is good, can you submit a patch in this ticket? It should probably go in tools.py with the other "/etc/hosts" functions.

For the cipher test issues, what are you OS/version running this on? Note - these are upstream tests so they should work F23 and up(anything else isn't really valid)

{{{
[spichugi@auto ~]$ cat /etc/redhat-release
Fedora release 24 (Twenty Four)
}}}
Also, we plan to run tests on downstream too. Will this part work there fine? Or should we skip some tests for RHEL 6 or RHEL 7, 71, 72?

This test is just going to be problematic moving forward. I'm wondering if we should just remove it? I got it working on F23/F24, but I feel this test is always going to need to be updated with each new release of NSS. For now lets get it working on F24(nss 3.25.0). I'll send you a diff I have for this test to include in your next patch.

Also, many of these tests are based on 389-ds-base-1.3.5 - so they should not be run against rhel 6 -> rhel 7.2 as they will most likely fail. Of course we have tests in 1.3.3 and 1.3.4, but these would also require a major overhaul to get them working correctly. Another issue with downstream testing is that not all the features/fixes that go into 1.3.5 will go into RHDS. So only "suites" should be used for downstream testing, not "tickets".

Replying to [comment:36 mreynolds]:

Hmmm, I had to remove "localhost.localdomain" and change it to 'localhost" to get these tests to pass on my jenkins server. Here is my hosts file from a F23 machine

{{{

cat /etc/hosts

127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
}}}

So your change would break things in my setup. I guess we need to find a better way to make this more portable. Does your hosts file start with "localhost.localdomain localhost"? Perhaps we should grab the the "local hostname" from /etc/hosts instead of hardcoding it?
/etc/hosts is broken for a long time: https://bugzilla.redhat.com/show_bug.cgi?id=1093037
Standard says FQDN should go first, then alias.
So in our beaker task for upstream tests we change it to the right order:
{{{
sed -e 's/^::1./::1 localhost.localdomain localhost/g' -e 's/^127.0.0.1./127.0.0.1 localhost.localdomain localhost/g' -i /etc/hosts
}}}
That's why it works in Simon's environment.

For the cipher test issues, what are you OS/version running this on? Note - these are upstream tests so they should work F23 and up(anything else isn't really valid)
In that case let's mark these tests with skipif fixture:
{{{
import pytest
from lib389.utils import ds_is_older
import lib389

@pytest.mark.skipif(ds_is_older("1.3.6"),
reason="DS does not support this")
def test_for_newer_ds():
pass
}}}

{{{
[0 root@qeos-229 upstream]# py.test -s -v test_skip_ds.py
========================================================== test session starts ==========================================================
platform linux2 -- Python 2.7.5, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 -- /usr/bin/python
cachedir: .cache
DS build: 1.3.5.10 B2016.197.39
389-ds-base: 1.3.5.10-5.el7
nss: 3.21.0-17.el7
nspr: 4.11.0-1.el7_2
openldap: 2.4.40-12.el7
svrcore: 4.1.2-1.el7

rootdir: /mnt/tests/rhds/tests/upstream, inifile:
plugins: beakerlib-0.5, html-1.9.0, cov-2.3.0
collected 1 items

test_skip_ds.py::test_for_newer_ds SKIPPED

======================================================= 1 skipped in 0.02 seconds =======================================================
}}}
Similarly we can add support for skipping based on nss version. We already querying for it in conftest.py for the header with the package versions.

attachment
0001-Ticket-48832-Fix-timing-and-localhost-issues.patch

attachment
0001-Ticket-48832-Add-DirSrvTools.getLocalhost-function.patch

Patches was added, please, review.

Regarding NSS. I've applied your diff. And if you want, I can work on the rewriting this test suite later (after my PTO).

Replying to [comment:40 spichugi]:

Patches was added, please, review.

Regarding NSS. I've applied your diff. And if you want, I can work on the rewriting this test suite later (after my PTO).

Sounds like a plan, thanks!

To ssh://git.fedorahosted.org/git/389/lib389.git

Pushed to master:
8e7893d..4b29192 master -> master
commit 4b29192c3ef34867d28d4ce3d308c5079b7ca5ae
Author: Simon Pichugin spichugi@redhat.com
Date: Fri Jul 29 17:34:48 2016 +0200

To ssh://git.fedorahosted.org/git/389/ds.git

Pushed to master:
d193997..6d472e4 master -> master
commit 6d472e4
Author: Simon Pichugin spichugi@redhat.com
Date: Fri Jul 29 13:40:41 2016 +0200

Fixes issues found from jenkins run
0001-Ticket-48832-Fix-CI-tests-failures-from-jenkins-serv.patch

Thanks for the review Noriko!

06a4adb..6f91ff5 master -> master
commit 6f91ff5
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Aug 11 16:19:56 2016 -0400

More test failures (timing related)
0001-Ticket-48832-Fix-more-CI-test-failures.patch

Thank you, Mark, for fixing the tests to run succssfully!

5fed802..bed1bd3 master -> master
commit bed1bd3
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Aug 19 11:21:24 2016 -0400

attachment
0001-Ticket-48832-Fix-CI-tests.patch

5223058..4d60ed2 master -> master
commit 4d60ed2
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Aug 31 12:19:15 2016 -0400

da91421..2b45d58 master -> master
commit 2b45d58

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1892

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.