Make testing on non-Linux platforms easier #141
Conversation
9a5c5fc to
6e929f1
Compare
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
- Coverage 69.33% 69.33% -0.01%
==========================================
Files 49 49
Lines 4654 4722 +68
Branches 782 802 +20
==========================================
+ Hits 3227 3274 +47
- Misses 1080 1093 +13
- Partials 347 355 +8
Continue to review full report at Codecov.
|
| HAVE_LDAPI = False | ||
| else: | ||
| HAVE_LDAPI = hasattr(socket, 'AF_UNIX') | ||
|
|
There was a problem hiding this comment.
What if, when CI is set, we only looked at CI_DISABLED? Maybe with an early error if LDAPI isn't disabled but socket.AF_UNIX is available.
And if CI is unset, do the hasattr check.
In this case, the hasattr is probably robust enough, but I think starting a "avoid feature detection if CI is set" pattern would be beneficial.
There was a problem hiding this comment.
I'd like to run tests without LDAPI support locally, too. Can you recommend a better way to pass an option from tox without introducing more env vars?
We could move to pytest in order to use proper fixtures and options. But I don't like to do that now.
There was a problem hiding this comment.
I misunderstood the code. Sorry for the noise!
Lib/slapdtest/_slapdtest.py
Outdated
| slapd_args = [ | ||
| self.PATH_SLAPD, | ||
| '-f', self._slapd_conf, | ||
| '-F', self.testrundir, | ||
| '-h', '%s' % ' '.join((self.ldap_uri, self.ldapi_uri)), | ||
| '-h', '%s' % ' '.join(urls), |
There was a problem hiding this comment.
The % interpolation looks unnecessary here.
Lib/slapdtest/_slapdtest.py
Outdated
| return name | ||
| return None | ||
|
|
||
|
|
There was a problem hiding this comment.
Here too, I feel it would be nice to avoid auto-discovery if CI is set.
There was a problem hiding this comment.
What do you mean by auto-detection here? _which simply looks through PATH.
Lib/slapdtest/_slapdtest.py
Outdated
| cmd = name[5:].lower() | ||
| raise ValueError( | ||
| "Command '{}' not found in PATH".format(cmd) | ||
| ) |
There was a problem hiding this comment.
So … commands are defined by being prefixed by PATH_?
That looks like avoidable magic. I think it would be better to have the explicit list and just add a comment to remind people about synchronization, or to turn _which into something that registers the command as well as finding it.
There was a problem hiding this comment.
The old list of file names doesn't result in good UX because _which returns None when it can't find a command. An error message like command None not found isn't very helpful. What do you suggest?
|
Should we turn on macOS on Travis, as well? |
LDAPI is LDAP over Unix Domain Socket, also referred as LDAP IPC. AF_UNIX is only supported on POSIX compatible operating systems. Don't bind to LDAPI socket and skip LDAPI specific tests, e.g. SASL EXTERNAL with SO_PEERCRED. Signed-off-by: Christian Heimes <cheimes@redhat.com>
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Instead of hard-coded paths /usr/bin and /usr/sbin, the SlapdObject test helper now uses PATH env var to find test binaries. For slapd server binary, sbin paths are automatically added to lookup PATH in case they are missing. Signed-off-by: Christian Heimes <cheimes@redhat.com>
Make SlapdObject.PATH_* instance attributes, rather than class ones. Set them in __init__. Move checking them from start() to __init__(), so the check becomes simply error handling. Put a straight-up copy of Python's shutil.which() in ldap.compat -- it is a temporary backport, not a modified fork.
|
Here is my suggestion: do both the I also made |
|
+1 for which() But I don't like the sbin code. It's too complicated. Should we really care where a command is located as long as it's found in a standard bin dir? I also don't like SLAPD, BINDIR and SBINDIR env vars. They are hacks that are no longer required with which. |
|
I agree they're too complicated. However, since 2.4.36, slapdtest is a top-level module for general use, rather than an internal python-ldap helper. I consider the env vars to be its public API. So, I don't want to drop them in 3.0. It might make sense to deprecate slapdtest as a general tool, and later make it an internal test helper again. |
|
I'm not fully satisfied with the patch, but it's as good as it will get. But it should do the trick while keeping backwards compatibility. Let's ship it! |
A bunch of small chances to the test suite. It should be enough to make it possible to run the test suite under macOS homebrew and Windows (untested).