attachment
0001-Ticket-48313-MEP-suite-tests-for-major-functionality.patch

Hi William,

thank you for such a great test coverage!

I have a few suggestions what we can improve.

First:
{{{
from mep_test_common import *

We need to name this import as it's private. Maybe this should be changed?

from mep_test_common import _mep_test_setup
}}}

You can change '''mep_test_common.py''', so there will be no need for the last import.

For that, you can rewrite your decorator

{{{
def _mep_test_setup(func):
def inner(topology):
# Make sure the ldap server is in a clean state, then populate it.
...
log.info('Test preparation complete')
func(topology)

return inner

}}}

to pytest.fixture:

{{{
@pytest.fixture(scope="module")
def mep_test_setup(topology):
# Make sure the ldap server is in a clean state, then populate it.
...
log.info('Test preparation complete')
}}}

After that, it is possible to remove:
{{{
from mep_test_common import _mep_test_setup
}}}

And use your new fixture within tests:
{{{
def test_mep_rdn(topology, mep_test_setup):
}}}

Also, it may be a good way to move your topology fixture declaration and log object creation from every test to '''mep_test_common.py'''.

After these actions, you will have to resolve object creation issues(some functions create objects within tests. For example, '''_mep_rdn_entry_entering_scope''' from '''mep_rdn_test.py''') and set up scope fixture parameter for '''topology''' and '''mep_test_setup''' properly. (it depends on you vision of your test suite, there is a many options)

Two another things are really minor:
- if it looks good for you, you can change this:

{{{
try:
topology.standalone.search_s(MEP_USER1_GROUP_DN, ldap.SCOPE_BASE, '(objectclass=top)')
assert False
except ldap.LDAPError as e:
assert True
}}}

to this, it has the same function:

{{{
with pytest.raises(ldap.LDAPError):
topology.standalone.search_s(MEP_USER1_GROUP_DN, ldap.SCOPE_BASE, '(objectclass=top)')
}}}

• as I see, '''mep_test.py''' does nothing, so may be we can delete it then?

Thanks,
Simon

to pytest.fixture:

{{{
@pytest.fixture(scope="module")
def mep_test_setup(topology):
# Make sure the ldap server is in a clean state, then populate it.
...
log.info('Test preparation complete')
}}}

There is method to my apparent madness: I need the mep_test_setup to be run before each of the inner parts of the test, not on the module as a whole. If you look at the actually test code you will start to see why - My test_func is a wrapper to smaller tests:

{{{
def test_mep_add(topology):
"""
Test function of MEP related to the addition of entries that will be managed.
"""

log.info('Test complete')

_mep_add_entry_out_of_scope(topology)
_mep_add_entry_in_scope(topology)

}}}

Between both of these tests, I need the contents of ou=People cleared out. If I ran this as a module fixture the code would be run as:

{{{

topology(request)
_mep_test_setup
test_mep_add
--> _mep_add_entry_out_of_scope
--> _mep_add_entry_in_scope
topology -fin
}}}

Which of course, will explode, because the entries already exist, so the test wouldn't work.

What I need is:

{{{

topology(request)
test_mep_add
--> _mep_test_setup
-----> _mep_add_entry_out_of_scope
---> _mep_test_setup
-----> _mep_add_entry_in_scope
topology -fin
}}}

I wanted to write this as a fixture, but py.test doesn't appear to have great docs on the matter to do with ordering of fixture application (Does the module fixture or the function level fixture get called first?) or even if you can do fixtures per test function, so I felt the safest option was to make it a decorator so that I can control the behavior. I would love to learn more about this and find a way to do this with fixtures, so if you know more about this, please do let me know!

Also, it may be a good way to move your topology fixture declaration and log object creation from every test to '''mep_test_common.py'''.

The issue is whether py.test will actually detect the fixture in the module and actually call it. I can test this and find out though, as I agree that duplication of code isn't the best thing to do.

Two another things are really minor:
- if it looks good for you, you can change this:

{{{
try:
topology.standalone.search_s(MEP_USER1_GROUP_DN, ldap.SCOPE_BASE, '(objectclass=top)')
assert False
except ldap.LDAPError as e:
assert True
}}}

to this, it has the same function:

{{{
with pytest.raises(ldap.LDAPError):
topology.standalone.search_s(MEP_USER1_GROUP_DN, ldap.SCOPE_BASE, '(objectclass=top)')
}}}

That looks much cleaner! I will refactor the relevant parts to match your suggestion here.

• as I see, '''mep_test.py''' does nothing, so may be we can delete it then?

I didn't add it, so I didn't want to go and delete it without checking. Given my new code, it's probably a good idea to remove mep_test.py, or have it as a wrapper that calls all 5 of the other modules perhaps.

William

attachment
0001-Ticket-48313-MEP-suite-tests-for-major-functionality.2.patch

This new version:

• Uses per-function fixtures (Turns out they work, and I can pass variables into them)
• Changes to the with pytest.raises pattern
• Refactors fixtures into mep_test_common
• Adds doc strings to all tests.
• Splits out all test items to be cleaner.

{{{
{root@7412c4ae5c5a 11:21} ~wibrown/development/389ds/ds I0> py.test dirsrvtests/suites/mep_plugin/mep_*_test.py
==================================================================================== test session starts =====================================================================================
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
collected 15 items

dirsrvtests/suites/mep_plugin/mep_add_test.py ..
dirsrvtests/suites/mep_plugin/mep_config_test.py ......
dirsrvtests/suites/mep_plugin/mep_del_test.py ..
dirsrvtests/suites/mep_plugin/mep_mod_test.py ..
dirsrvtests/suites/mep_plugin/mep_rdn_test.py ...
}}}

Hi William,

great work! I'm enjoyed your test suite.

Two really minor things:
- you can freely remove this from every test case, except '''mep_test_common.py''':
{{{
logging.getLogger(name).setLevel(logging.DEBUG)
log = logging.getLogger(name)
}}}
It will be enough and everything will still work.
- '''mep_test_common.py''' contains the line:

{{{
141 print(entry.dn)
}}}
Is it necessary? As an option, we can put it to the log.info() function.

The rest looks very good for me.
Thank you.

Simon

attachment
0001-Ticket-48313-MEP-suite-tests-for-major-functionality.3.patch

Applied both of Simon's recommendations. Thanks for reviewing this once again.

Looks good for me. Thank you, William.

Pushed to master
master -> be1460bde6016834df7cb74b51fb26dc38abd126

Thanks for your help Simon!

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/1644

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.