diff --git a/chapter_03_abstractions.asciidoc b/chapter_03_abstractions.asciidoc index d4bbf4f2..9af9e936 100644 --- a/chapter_03_abstractions.asciidoc +++ b/chapter_03_abstractions.asciidoc @@ -196,8 +196,8 @@ def sync(source, dest): # for every file that appears in source but not target, copy the file to # the target - for src_hash, fn in source_hashes.items(): - if src_hash not in seen: + for source_hash, fn in source_hashes.items(): + if source_hash not in seen: shutil.copy(Path(source) / fn, Path(dest) / fn) ---- ==== @@ -353,14 +353,14 @@ what _abstraction_ of filesystem actions will happen?" [role="skip"] ---- def test_when_a_file_exists_in_the_source_but_not_the_destination(): - src_hashes = {'hash1': 'fn1'} - dst_hashes = {} + source_hashes = {'hash1': 'fn1'} + dest_hashes = {} expected_actions = [('COPY', '/src/fn1', '/dst/fn1')] ... def test_when_a_file_has_been_renamed_in_the_source(): - src_hashes = {'hash1': 'fn1'} - dst_hashes = {'hash1': 'fn2'} + source_hashes = {'hash1': 'fn1'} + dest_hashes = {'hash1': 'fn2'} expected_actions == [('MOVE', '/dst/fn2', '/dst/fn1')] ... ---- @@ -411,11 +411,11 @@ def sync(source, dest): # imperative shell step 3, apply outputs for action, *paths in actions: - if action == "copy": + if action == "COPY": shutil.copyfile(*paths) - if action == "move": + if action == "MOVE": shutil.move(*paths) - if action == "delete": + if action == "DELETE": os.remove(paths[0]) ---- ==== @@ -453,21 +453,21 @@ structures: ==== [source,python] ---- -def determine_actions(src_hashes, dst_hashes, src_folder, dst_folder): - for sha, filename in src_hashes.items(): - if sha not in dst_hashes: - sourcepath = Path(src_folder) / filename - destpath = Path(dst_folder) / filename - yield "copy", sourcepath, destpath - - elif dst_hashes[sha] != filename: - olddestpath = Path(dst_folder) / dst_hashes[sha] - newdestpath = Path(dst_folder) / filename - yield "move", olddestpath, newdestpath - - for sha, filename in dst_hashes.items(): - if sha not in src_hashes: - yield "delete", dst_folder / filename +def determine_actions(source_hashes, dest_hashes, source_folder, dest_folder): + for sha, filename in source_hashes.items(): + if sha not in dest_hashes: + sourcepath = Path(source_folder) / filename + destpath = Path(dest_folder) / filename + yield "COPY", sourcepath, destpath + + elif dest_hashes[sha] != filename: + olddestpath = Path(dest_folder) / dest_hashes[sha] + newdestpath = Path(dest_folder) / filename + yield "MOVE", olddestpath, newdestpath + + for sha, filename in dest_hashes.items(): + if sha not in source_hashes: + yield "DELETE", dest_folder / filename ---- ==== @@ -480,16 +480,16 @@ Our tests now act directly on the `determine_actions()` function: [source,python] ---- def test_when_a_file_exists_in_the_source_but_not_the_destination(): - src_hashes = {"hash1": "fn1"} - dst_hashes = {} - actions = determine_actions(src_hashes, dst_hashes, Path("/src"), Path("/dst")) + source_hashes = {"hash1": "fn1"} + dest_hashes = {} + actions = determine_actions(source_hashes, dest_hashes, Path("/src"), Path("/dst")) assert list(actions) == [("copy", Path("/src/fn1"), Path("/dst/fn1"))] def test_when_a_file_has_been_renamed_in_the_source(): - src_hashes = {"hash1": "fn1"} - dst_hashes = {"hash1": "fn2"} - actions = determine_actions(src_hashes, dst_hashes, Path("/src"), Path("/dst")) + source_hashes = {"hash1": "fn1"} + dest_hashes = {"hash1": "fn2"} + actions = determine_actions(source_hashes, dest_hashes, Path("/src"), Path("/dst")) assert list(actions) == [("move", Path("/dst/fn2"), Path("/dst/fn1"))] ---- ==== @@ -529,34 +529,33 @@ system together but fake the I/O, sort of _edge to edge_: [source,python] [role="skip"] ---- -def sync(reader, filesystem, source_root, dest_root): #<1> +def sync(source, dest, filesystem=FileSystem()): #<1> + source_hashes = filesystem.read(source) #<2> + dest_hashes = filesystem.read(dest) #<2> - source_hashes = reader(source_root) #<2> - dest_hashes = reader(dest_root) - - for sha, filename in src_hashes.items(): + for sha, filename in source_hashes.items(): if sha not in dest_hashes: - sourcepath = source_root / filename - destpath = dest_root / filename - filesystem.copy(destpath, sourcepath) #<3> + sourcepath = Path(source) / filename + destpath = Path(dest) / filename + filesystem.copy(sourcepath, destpath) #<3> elif dest_hashes[sha] != filename: - olddestpath = dest_root / dest_hashes[sha] - newdestpath = dest_root / filename - filesystem.move(olddestpath, newdestpath) + olddestpath = Path(dest) / dest_hashes[sha] + newdestpath = Path(dest) / filename + filesystem.move(olddestpath, newdestpath) #<3> - for sha, filename in dst_hashes.items(): + for sha, filename in dest_hashes.items(): if sha not in source_hashes: - filesystem.delete(dest_root/filename) + filesystem.delete(dest / filename) #<3> ---- ==== -<1> Our top-level function now exposes two new dependencies, a `reader` and a - `filesystem`. +<1> Our top-level function now exposes a new dependency, a `FileSystem`. -<2> We invoke the `reader` to produce our files dict. +<2> We invoke `filesystem.read()` to produce our files dict. -<3> We invoke the `filesystem` to apply the changes we detect. +<3> We invoke the ++FileSystem++'s `.copy()`, `.move()` and `.delete()` methods + to apply the changes we detect. TIP: Although we're using dependency injection, there is no need to define an abstract base class or any kind of explicit interface. In this @@ -566,24 +565,81 @@ TIP: Although we're using dependency injection, there is no need // IDEA [KP] Again, one could mention PEP544 protocols here. For some reason, I like them. -[[bob_tests]] +The real (default) implementation of our FileSystem abstraction does real I/O: + +[[real_filesystem_wrapper]] +.The real dependency (sync.py) +==== +[source,python] +[role="skip"] +---- +class FileSystem: + + def read(self, path): + return read_paths_and_hashes(path) + + def copy(self, source, dest): + shutil.copyfile(source, dest) + + def move(self, source, dest): + shutil.move(source, dest) + + def delete(self, dest): + os.remove(dest) +---- +==== + +But the fake one is a wrapper around our chosen abstractions, +rather than doing real I/O: + +[[fake_filesystem]] .Tests using DI ==== [source,python] [role="skip"] ---- -class FakeFileSystem(list): #<1> +class FakeFilesystem: + def __init__(self, path_hashes): #<1> + self.path_hashes = path_hashes + self.actions = [] #<2> + + def read(self, path): + return self.path_hashes[path] #<1> - def copy(self, src, dest): #<2> - self.append(('COPY', src, dest)) + def copy(self, source, dest): + self.actions.append(('COPY', source, dest)) #<2> - def move(self, src, dest): - self.append(('MOVE', src, dest)) + def move(self, source, dest): + self.actions.append(('MOVE', source, dest)) #<2> def delete(self, dest): - self.append(('DELETE', dest)) + self.actions.append(('DELETE', dest)) #<2> +---- +==== + +<1> We initialize our fake filesysem using the abstraction we chose to + represent filesystem state: dictionaries of hashes to paths. + +<2> The action methods in our `FakeFileSystem` just appends a record to an list + of `.actions` so we can inspect it later. This means our test double is both + a "fake" and a "spy". + ((("test doubles"))) + ((("fake objects"))) + ((("spy objects"))) + +So now our tests can act on the real, top-level `sync()` entrypoint, +but they do so using the `FakeFilesystem()`. In terms of their +setup and assertions, they end up looking quite similar to the ones +we wrote when testing directly against the functional core `determine_actions()` +function: +[[bob_tests]] +.Tests using DI +==== +[source,python] +[role="skip"] +---- def test_when_a_file_exists_in_the_source_but_not_the_destination(): source = {"sha1": "my-file" } dest = {} @@ -607,15 +663,6 @@ def test_when_a_file_has_been_renamed_in_the_source(): ---- ==== -<1> Bob _loves_ using lists to build simple test doubles, even though his - coworkers get mad. It means we can write tests like - ++assert 'foo' not in database++. - ((("test doubles", "using lists to build"))) - -<2> Each method in our `FakeFileSystem` just appends something to the list so we - can inspect it later. This is an example of a spy object. - ((("spy objects"))) - The advantage of this approach is that our tests act on the exact same function that's used by our production code. The disadvantage is that we have to make @@ -743,9 +790,12 @@ Steve Freeman has a great example of overmocked tests in his talk https://oreil.ly/jAmtr["Test-Driven Development"]. You should also check out this PyCon talk, https://oreil.ly/s3e05["Mocking and Patching Pitfalls"], by our esteemed tech reviewer, Ed Jung, which also addresses mocking and its -alternatives. And while we're recommending talks, don't miss Brandon Rhodes talking about -https://oreil.ly/oiXJM["Hoisting Your I/O"], -which really nicely covers the issues we're talking about, using another simple example. +alternatives. + +And while we're recommending talks, check out the wonderful Brandon Rhodes +in https://oreil.ly/oiXJM["Hoisting Your I/O"]. It's not actually about mocks, +but is instead about the general issue of decoupling business logic from I/O, +in which he uses a wonderfully simple illustrative example. ((("hoisting I/O"))) ((("Rhodes, Brandon"))) @@ -792,12 +842,17 @@ a few heuristics and questions to ask yourself: messy system and then try to imagine a single function that can return that state? -* Where can I draw a line between my systems, where can I carve out a - https://oreil.ly/zNUGG[seam] to stick that abstraction in? +* Separate the _what_ from the _how_: + can I use a data structure or DSL to represent the external effects I want to happen, + independently of _how_ I plan to make them happen? + +* Where can I draw a line between my systems, + where can I carve out a https://oreil.ly/zNUGG[seam] + to stick that abstraction in? ((("seams"))) -* What is a sensible way of dividing things into components with different - responsibilities? What implicit concepts can I make explicit? +* What is a sensible way of dividing things into components with different responsibilities? + What implicit concepts can I make explicit? * What are the dependencies, and what is the core business logic?