If we have this error then something is wrong and there is no need to continue the execution. I am not sure why we catch the exception here and only log it (and then we continue the execution)

I think run_healthcheck and run_healthcheck_with_json can be easily merged and with_json will be just a parameter

You can use format_cmd_list(cmdline) from utils.py for better formatting.

This env is picked up in the topology, you don't need it here IIRC.

I'm pretty concerned about the structure of this test. An important aspect of how lib389 was structured is that you should be able to test parts in isolation - this test seems to break that, relying on calls to dscreate and dsctl directly. What happenes on a prefix install where dscreate is not in /bin? What happens if that version mismatches the developer instance? I think this structure really makes it more fragile.

There is a cli testing framework that allows you to test individual commands from dsctl/dsconf/dsidm, that does not require shelling out, or subprocess, and can have instances passed in, but no one seems to know about it or use it. See here:

https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/tests/cli/idm_group_test.py

Sadly I think because it's not known abotu it,s been a bit neglected, but it's designed to:

• Capture all command output for comparison in check
• Allow creation of instances and commands of various structures.
• To not need to rely on shell or fragile environment configurations.

I'd really urge you to consider swapping over to it. This is an example of how to structure a dsctl test:

https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/tests/cli/ctl_dbtasks_test.py

@spichugi
Changes done as per suggestion, please review. Thanks!

@firstyear
Thank you for this suggestion. We mostly agreed to use subprocess because we want to test it on instance created from scratch and we need these tests done for reasons of time. But I will keep this in mind and I can convert the tests to framework later.

@spichugi
Changes done as per suggestion, please review. Thanks!
@firstyear
Thank you for this suggestion. We mostly agreed to use subprocess because we want to test it on instance created from scratch and we need these tests done for reasons of time.

Yes, but also please remember that upstream is not redhat internal ...

But I will keep this in mind and I can convert the tests to framework later.

Quick fixes, or "we'll convert later" never actually happens, ever.

There is literally a cli testing framework that exists for this purpose, does exactly what you need. It can create instances from scratch and even more. I ask you to please use it.

@firstyear
Ok, I will try to convert it to your suggested framework and see how it goes :)

Thank you - I think you'll find it will help you a lot to resolve issues and create tests. If you need more resources or help on this, please write here or email me (wbrown at suse.de)

Hey @firstyear, can you confirm that this framework actually works for you on the latest master?
Because it doesn't work with Python installer unless I do this:

diff --git a/src/lib389/lib389/tests/cli/__init__.py b/src/lib389/lib389/tests/cli/__init__.py
index 56da03a0b..51b3015bb 100644
--- a/src/lib389/lib389/tests/cli/__init__.py
+++ b/src/lib389/lib389/tests/cli/__init__.py
@@ -35,7 +35,7 @@ def topology_be_latest(request):
     topology = create_topology({ReplicaRole.STANDALONE: 1}, None)
     topology.standalone.backends.create(properties={
         'cn': 'userRoot',
-        'suffix': DEFAULT_SUFFIX,
+        'nsslapd-suffix': DEFAULT_SUFFIX,
     })
     # Now apply sample entries
     centries = get_sample_entries(INSTALL_LATEST_CONFIG)
@@ -58,7 +58,7 @@ def topology_be_001003006(request):
     topology = create_topology({ReplicaRole.STANDALONE: 1}, None)
     topology.standalone.backends.create(properties={
         'cn': 'userRoot',
-        'suffix': DEFAULT_SUFFIX,
+        'nsslapd-suffix': DEFAULT_SUFFIX,
     })
     # Now apply sample entries
     centries = get_sample_entries('001003006')

And it doesn't work with Perl installer at all, because it always expects some suffix to be created.

Also I don't see why these topologies have to live in a separate module. We can extend existing module instead of spreading several different approaches across lib389.

Errghh, yeah, it's because the cli parameters changed. I guess the test is showing you that it changed ....

Yes, it won't work with perl at all, it's python only.

It has a seperate topology because it hooks the dscreate front end api, rather than calling the python setup directly.

@firstyear
So I am getting a grasp of how the framework works but we found issue https://pagure.io/389-ds-base/issue/51019
This causes a problem when running healthcheck, because it will prompt you for bindDN and password and the tests will hang.

@firstyear
So I am getting a grasp of how the framework works but we found issue https://pagure.io/389-ds-base/issue/51019
This causes a problem when running healthcheck, because it will prompt you for bindDN and password and the tests will hang.

The problem is that lib389 topologies are still using the older installer(setup-ds.pl). We really need to get off of the legacy tools in lib389.

The problem is that lib389 topologies are still using the older installer(setup-ds.pl). We really need to get off of the legacy tools in lib389.

This is not true, we can swap between python and perl installer on the fly (via defaults.inf).
Even when python installer is used, it doesn't populate the same options as we use in dscreate.

I take that back. I tried once again with the python installer and ldapi works...

No, the topologies are agnostic to this, if defaults.inf has perl because you compiled with perl on the install, then dirsrv.create detects that and will use it. So ... don't compile with perl?

@vashirov
Can you please review if that is how we wanted to move the topologies? (So we don't use those in src/lib389/lib389/tests/cli/init.py)
And also the usage of env variable PYINSTALL.
Thanks!

This looks heaps better with the fake args by the way. I'll let @vashirov still check, but I'm much happier with this!

This indeed looks much better!

Few suggestions:
PYINSTALL unlike DEBUGGING is boolean. For DEBUGGING we planned to add multiple debug levels, but it's not there yet, so we just check for its presence, but not much else.
So with PYINSTALL we can do something like this:

PYINSTALL = True if os.getenv('PYINSTALL') else False

And then just pass PYINSTALL value everywhere for instance creation and deletion:

instance.create(pyinstall=PYINSTALL)
...
topology.standalone.delete(pyinstall=PYINSTALL)

So if PYINSTALL is present in the environment, python installer would be used. If not, lib389 will fallback to enable_perl value in defaults.inf and continue with either python or perl installer.

Please make sure you pass pyinstall to delete() everywhere, as of right now it stays on the system after the test.

Thanks!

@vashirov
Changed the PYINSTALL variable, please review.
Thanks!

@vashirov
I added the pytestmark.skip to blank tests.

Please add a check for 'not ds_paths.perl_enabled' too, so we can run these tests if the package is built without perl and not using PYINSTALL explicitly.

Besides that, LGTM!

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3956

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.