Codecov Report

Merging #198 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   70.52%   70.62%   +0.09%     
==========================================
  Files          49       49              
  Lines        4808     4806       -2     
  Branches      811      811              
==========================================
+ Hits         3391     3394       +3     
+ Misses       1082     1078       -4     
+ Partials      335      334       -1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adc40d0...ca705aa. Read the comment docs.

slapdtest is an internal helper for python-ldap tests. I'm worried about adding features that aren't used in the tests.
Specifically, there are no API guarantees: if you use slapdtest elsewhere, we might break your code at any time. (But of course, you're welcome to fix any breakage if the fix makes sense for python-ldap as well.)
Are you OK with that?

I understand. I'm ok with that, yes.

FWIW, I find slapdtest extremely convenient and useful to test my application's LDAP interactions. I see a lot of value in it. The module is documented, so its use is encouraged to some degree.

Would you like me to add a test? I don't mind adding it and will help make sure the API continues to work.

The main thing I'm worried about is feature creep: I'm not sure what the scope of slapdtest should be.
If it's an internal test helper for python-ldap, then the scope is clear – whatever python-ldap needs. I didn't have a problem adding the cert support, for example.
But if it's to be a generic library, we need to somehow define boundaries: what slapdtest is and what it isn't. I'd be fine with defining the scope informally, even as an idea floating in the maintainer's head. But currently, I don't have that :)
For example, should slapdtest be useful without python-ldap? Should it have its own tests? In that case, it would make sense to turn it into a separate project (still under the python-ldap organization).

This particular change is fine; I'll merge it next time I get a block of time for python-ldap.
As long as slapdtest is as an internal helper, it's enough that python-ldap tests pass, no need for an extra test.

I share Petr's opinion. For me, the slapdtest package is an internal test helper for python-ldap. We should document the fact, that slapdtest doesn't have the same API stability as the rest of the package.

I see two problems with your PR

1. There is no test case for the context manager protocol. The method are not executed in any test.
2. The behavior of __enter__ is undefined when start was already called.

In that case, it would make sense to turn it into a separate project (still under the python-ldap organization).

If you choose to take this direction, I'm OK with that. Here is an example of another project using slapdtest to greatly improve the robustness of its test suite: django-auth-ldap/django-auth-ldap@d72144a

From @encukou

no need for an extra test.

From @tiran

There is no test case for the context manager protocol. The method are not executed in any test.

I'm still happy to write a test if it is desired, but I'm getting a mixed message on whether or not it is desired.

The behavior of __enter__ is undefined when start was already called.

It behaves exactly the same as calling start() twice. Meaning, the second call will not start a second slapd server due to the if _proc is None guard. When the context manager exits, the server is always stopped. (I can capture this behavior in a test as well.)

I have updated the PR to include:

• A test for the context manager
• A test for the context manager after calling .start()
• Added documentation about using the context manager

In that case, it would make sense to turn it into a separate project (still under the python-ldap organization).

If you choose to take this direction. I'm OK with that. Here is an example of another project using slapdtest to greatly improve the robustness of its test suite: django-auth-ldap/django-auth-ldap@d72144a

OK. Let's go in that direction. #203

I'm still happy to write a test if it is desired, but I'm getting a mixed message on whether or not it is desired.

Always better to have a test! It's not absolutely required if we think of sldaptest as only an internal testing tool.

In code I'd write today, anything that could have start and stop methods would be a context manager first. With explicit start/stop calls it's too easy to forget the clean-up.
Of course, unittest, coming from an age when we didn't have context managers, is built around explicit setup/teardown calls. That's fine, no need to rebuild the world when something better/safer/easier comes along, but it's no reason to not add support for the new thing.

I want to merge this. @tiran, do you want to comment?