This repository was archived by the owner on Jan 27, 2019. It is now read-only.
add "oe test" subcommand #217
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've tried to add small unit tests here and there, especially when
low-level OS stuff is involved. This is an attempt at creating a place
to put all these, allowing them to be run without weird and ad hoc
command lines.
The first test case I'm adding is a bit complex, but it's the simplest I
could come up with to provoke the race in our makedirs helper. That race
may show its ugly face when we do stuff in parallel processes and be
very hard to debug ("what do you mean EEXIST, the code obviously tests
for that before calling os.makedirs!"). Before fixing any race one needs
a test case that reliably reproduces it.
The stdlib utility function os.makedirs raises EEXIST if the leaf directory already exists. Our makedirs helper tries to avoid that by checking for that condition upfront. However, when doing tasks in parallel, two different processes may call makedirs concurrently; both may initially see that the directory does not exist, one will successfully call os.makedirs(), while the other will get an EEXIST exception. Debugging that would be no fun, since the code clearly checks whether the path exists... This reimplements the helper to avoid such races. The previous implementation would also silently return success in the case where 'path' exists and is a regular file, which would in all likelyhood just lead to ENOTDIR later when the caller tries to create 'path + "/foo"'. It's better to fail early, and this is also consistent with 'mkdir -p' failing if the target exists but is not a directory.
Maybe overkill, but it's nice to know what one can expect.
This gives it a better chance of actually getting run once in a while. Should we ever go for Py3 (3.2+, https://docs.python.org/3.2/library/subprocess.html), oelite.signal.restore_defaults becomes redundant but harmless.
We get the sha1 or md5 digest of files in a couple of places by doing
m = hashlib.md5()
with open("foo") as f:
m.update(f.read())
h = m.hexdigest()
But that .read() call can be rather expensive if the file in question is
a 100M tarball (opencv, I'm looking at you); it temporarily bumps our
memory usage quite a lot, and it's also terribly inefficient (lots of
page faults and completely blowing the cpu cache and TLB first by
populating the gigantic buffer, then again when reading through it).
So create a generator which simply yields a given file in chunks. Then
the above would be:
m = hashlib.md5()
for chunk in iter_chunks("foo"):
m.update(chunk)
h = m.hexdigest()
but since we do that middle part a lot, create another helper that does
that and returns the passed-in hasher (we sometimes feed more than just
one file to the same hasher, so we don't finish up by calling
.hexdigest). Then the above could be simply
h = oelite.util.hash_file(hashlib.md5(), "foo").hexdigest()
This makes signature verification (hence do_fetch) somewhat cheaper for large ingredients tar balls (I've measured about 10x for the largest ones).
The last calls of the write_checksum and verify_checksum methods vanished with 6f3677d (class/fetch, lib/oelite: Improve fetch support). That commit also introduced the cache() method, but as far as I can tell, that has never had a caller; moreover, no fetcher has ever implemented it, so it's not really clear what its semantics should be.
As further prep for parallel do_fetch / do_unpack, don't open-code the old and racy version of oelite.util.makedirs.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on trying to allow async do_fetch and other python functions, I've realized I'm gonna need some low-level helpers (lockfiles, some signal handling), and that stuff is tricky enough that I want to be able to test those directly. Our makedirs helper also turned out to be racy, so the first test case is rather complex.
While at it, move the tiny existing self tests to 'oe test' so that they can get run without odd python invocations. Also, I use this opportunity to add a little performance improvement to do_fetch; being able to write a test for the new helper in just a few lines makes me much more confident that the new code really is equivalent to the old.
I also plan to add some sort of unit tests of our various task functions (for example, to allow refactoring do_split while being confident that the semantics don't change, and if they do, that it's deliberate and documented in the commit message why), but that is likely to be in the form of special-purpose recipes.