)]}'
{"nova/tests/unit/virt/libvirt/fake_imagebackend.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"6dd6f09856e92dcd8adf20dc9ba4689c5799a0b2","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    def cache(self, fetch_func, filename, size\u003dNone, *args, **kwargs):"},{"line_number":35,"context_line":"        pass"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def snapshot(self, name):"},{"line_number":38,"context_line":"        pass"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def libvirt_info(self, disk_bus, disk_dev, device_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3aaa91ec_79fc05b5","line":37,"updated":"2016-07-01 10:07:43.000000000","message":"This method should be removed, no?","commit_id":"2bb68e34102590a060d8bc3dedbea46efb3bb32a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8c3653021ad63ce3c774f0121c684bfc3bccfae8","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    def cache(self, fetch_func, filename, size\u003dNone, *args, **kwargs):"},{"line_number":35,"context_line":"        pass"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def snapshot(self, name):"},{"line_number":38,"context_line":"        pass"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def libvirt_info(self, disk_bus, disk_dev, device_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_ddbb2a0d","line":37,"in_reply_to":"3aaa91ec_79fc05b5","updated":"2016-07-04 12:03:52.000000000","message":"No, this is the snapshot method of Image, not the (confusingly named) snapshot method of imagebackend.Backend.\n\nSee I6ab8eb3b where I fixup the other snapshot method.","commit_id":"2bb68e34102590a060d8bc3dedbea46efb3bb32a"},{"author":{"_account_id":2506,"name":"devdatta-kulkarni","email":"kulkarni.devdatta@gmail.com","username":"devdatta-kulkarni"},"change_message_id":"263cf3f4fe477981bcf56a55faded4f82cbdc121","unresolved":false,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        return FakeImage(instance, name)"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def snapshot(self, instance, disk_path, image_type\u003d\u0027\u0027):"},{"line_number":59,"context_line":"        # NOTE(bfilippov): this is done in favor for"},{"line_number":60,"context_line":"        # snapshot tests in test_libvirt.LibvirtConnTestCase"},{"line_number":61,"context_line":"        return imagebackend.Backend(True).snapshot(instance,"}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_0c71e2a8","side":"PARENT","line":58,"updated":"2016-08-03 19:37:54.000000000","message":"Curious as to why this was removed?","commit_id":"c5ddf7f853f3b8ec4c990b2bafc7a148515b1bae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a23f1d24cec7ce95688e482020fb79c1bb4dc7f6","unresolved":false,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        return FakeImage(instance, name)"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def snapshot(self, instance, disk_path, image_type\u003d\u0027\u0027):"},{"line_number":59,"context_line":"        # NOTE(bfilippov): this is done in favor for"},{"line_number":60,"context_line":"        # snapshot tests in test_libvirt.LibvirtConnTestCase"},{"line_number":61,"context_line":"        return imagebackend.Backend(True).snapshot(instance,"}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_cf7cb8b7","side":"PARENT","line":58,"in_reply_to":"bacf61ea_0c71e2a8","updated":"2016-08-04 12:26:01.000000000","message":"The new version fakes the class returned by backend() instead, so faking this isn\u0027t required. It\u0027s still there, because it\u0027s implemented by the base class.","commit_id":"c5ddf7f853f3b8ec4c990b2bafc7a148515b1bae"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":30,"context_line":"        \"\"\"This fixture mocks imagebackend.Backend.backend, which is the"},{"line_number":31,"context_line":"        only entry point to libvirt.imagebackend from libvirt.driver."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"        :param gotFiles: A list of {\u0027filename\u0027: path, \u0027size\u0027: size} for every"},{"line_number":34,"context_line":"                         file which was created."},{"line_number":35,"context_line":"        :param imported_files: A list of (local_filename, remote_filename) for"},{"line_number":36,"context_line":"                               every invocation of import_file()."}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_92a07d92","line":33,"updated":"2016-08-15 14:44:28.000000000","message":"This is new code, nothing existing is calling this init function, so you can avoid the camelCase argument name at least here, right?","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9eb6286ddf17583f2506dc22ad0b811289497c37","unresolved":false,"context_lines":[{"line_number":30,"context_line":"        \"\"\"This fixture mocks imagebackend.Backend.backend, which is the"},{"line_number":31,"context_line":"        only entry point to libvirt.imagebackend from libvirt.driver."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"        :param gotFiles: A list of {\u0027filename\u0027: path, \u0027size\u0027: size} for every"},{"line_number":34,"context_line":"                         file which was created."},{"line_number":35,"context_line":"        :param imported_files: A list of (local_filename, remote_filename) for"},{"line_number":36,"context_line":"                               every invocation of import_file()."}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_bd72a5e3","line":33,"in_reply_to":"3ac371cc_7435e944","updated":"2016-08-15 16:37:58.000000000","message":"I know there are, but this adds no code that is referenced by existing code, so the only \"confusing\" part is a few places like this:\n\n got_files\u003dgotFiles\n\nwhich I don\u0027t think is confusing, especially for a few patches of lifetime. However, I\u0027m going to ignore this since it\u0027ll just create conflicts in the patch that removes it.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":30,"context_line":"        \"\"\"This fixture mocks imagebackend.Backend.backend, which is the"},{"line_number":31,"context_line":"        only entry point to libvirt.imagebackend from libvirt.driver."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"        :param gotFiles: A list of {\u0027filename\u0027: path, \u0027size\u0027: size} for every"},{"line_number":34,"context_line":"                         file which was created."},{"line_number":35,"context_line":"        :param imported_files: A list of (local_filename, remote_filename) for"},{"line_number":36,"context_line":"                               every invocation of import_file()."}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_7435e944","line":33,"in_reply_to":"3ac371cc_92a07d92","updated":"2016-08-15 16:15:46.000000000","message":"There are existing tests which have an existing list called gotFiles. You\u0027re right, I could rename it here and the caller would still call it gotFiles, but I think this is less confusing, especially as it won\u0027t live long anyway.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":31,"context_line":"        only entry point to libvirt.imagebackend from libvirt.driver."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"        :param gotFiles: A list of {\u0027filename\u0027: path, \u0027size\u0027: size} for every"},{"line_number":34,"context_line":"                         file which was created."},{"line_number":35,"context_line":"        :param imported_files: A list of (local_filename, remote_filename) for"},{"line_number":36,"context_line":"                               every invocation of import_file()."},{"line_number":37,"context_line":"        :param exists: An optional lambda which takes the disk name as an"}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_eff2ae99","line":34,"updated":"2016-08-15 14:44:28.000000000","message":"For a fixture, \"was\" is a strange tense to use to describe a parameter to the constructor. In fact, all of the descriptions of these parameters are confusing to me. I think they describe \"things that will be returned\" or \"things that represent the state of the system\", etc, but they\u0027re really hard to understand. I think that comes from lifting this code out of very a specific place and trying to generalize it. Perhaps you could have a read and another go at making these a little more general? I definitely had to read the code in the fixture to know what they were doing..","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":31,"context_line":"        only entry point to libvirt.imagebackend from libvirt.driver."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"        :param gotFiles: A list of {\u0027filename\u0027: path, \u0027size\u0027: size} for every"},{"line_number":34,"context_line":"                         file which was created."},{"line_number":35,"context_line":"        :param imported_files: A list of (local_filename, remote_filename) for"},{"line_number":36,"context_line":"                               every invocation of import_file()."},{"line_number":37,"context_line":"        :param exists: An optional lambda which takes the disk name as an"}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_d4f93d4f","line":34,"in_reply_to":"3ac371cc_eff2ae99","updated":"2016-08-15 16:15:46.000000000","message":"I\u0027d say don\u0027t worry about gotFiles and imported_files too much. Their days are numbered, and they\u0027re purely to interoperate with existing tests. The alternative is that I could rewrite all tests which use them in this change, but I don\u0027t think it\u0027s worth the effort or the noise.\n\nI don\u0027t want to make them general, because I want to remove them. The general solution is the disks dict.\n\nIf you *really, really* won\u0027t have them, I\u0027ll also rewrite the tests which use them. That\u0027s not the end of the world, but I\u0027d prefer to do it incrementally.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":6608,"name":"Paul Carlton","email":"paul.carlton2@hpe.com","username":"paul-carlton2"},"change_message_id":"163d4d0fc60bf3efc73b5b9c583f58e1b19fc456","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.imported_files \u003d imported_files"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        self.disks \u003d collections.defaultdict(self._mock_disk)"},{"line_number":45,"context_line":"        \"\"\"A dict of name -\u003e Mock image object. This is a defaultdict,"},{"line_number":46,"context_line":"        so tests may access it directly before a disk has been created.\"\"\""},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"        self._exists \u003d exists"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ac371cc_d8ad48ea","line":45,"updated":"2016-08-18 09:32:57.000000000","message":"I personally think this would look better as two comment lines, i.e. \n\n# A dict of name -\u003e Mock image object. This is a defaultdict,\n# so tests may access it directly before a disk has been created.\n\nbut just a nit","commit_id":"32be7a4a028d19a9fe929db20b6eb637f597175c"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f44ce9b6974b9b9294ab54355a874076e2f986a3","unresolved":false,"context_lines":[{"line_number":42,"context_line":"        self.imported_files \u003d imported_files"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        self.disks \u003d collections.defaultdict(self._mock_disk)"},{"line_number":45,"context_line":"        \"\"\"A dict of name -\u003e Mock image object. This is a defaultdict,"},{"line_number":46,"context_line":"        so tests may access it directly before a disk has been created.\"\"\""},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"        self._exists \u003d exists"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ac371cc_a1763e03","line":45,"in_reply_to":"3ac371cc_d8ad48ea","updated":"2016-08-18 14:57:49.000000000","message":"I\u0027m in 2 minds about this, because disks is part of the public interface of this fixture. I know nobody ever looks at the generated docs, but that was my thought process.","commit_id":"32be7a4a028d19a9fe929db20b6eb637f597175c"},{"author":{"_account_id":6608,"name":"Paul Carlton","email":"paul.carlton2@hpe.com","username":"paul-carlton2"},"change_message_id":"163d4d0fc60bf3efc73b5b9c583f58e1b19fc456","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        # When the \u0027constructor\u0027 (image_init in _mock_backend) later runs,"},{"line_number":80,"context_line":"        # it will return the same object we created here, and when the"},{"line_number":81,"context_line":"        # caller calls cache() it will raise the requested exception."},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        disk \u003d mock.create_autospec(imagebackend.Image)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"        # NOTE(mdbooth): fake_cache and fake_import_file are for compatiblity"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ac371cc_b857acc8","line":82,"updated":"2016-08-18 09:32:57.000000000","message":"for consistency shouldn\u0027t this be in \"\"\" ... \"\"\"\"?\nbut again just a nit","commit_id":"32be7a4a028d19a9fe929db20b6eb637f597175c"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f44ce9b6974b9b9294ab54355a874076e2f986a3","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        # When the \u0027constructor\u0027 (image_init in _mock_backend) later runs,"},{"line_number":80,"context_line":"        # it will return the same object we created here, and when the"},{"line_number":81,"context_line":"        # caller calls cache() it will raise the requested exception."},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        disk \u003d mock.create_autospec(imagebackend.Image)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"        # NOTE(mdbooth): fake_cache and fake_import_file are for compatiblity"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ac371cc_ec282d88","line":82,"in_reply_to":"3ac371cc_b857acc8","updated":"2016-08-18 14:57:49.000000000","message":"Similar to disks, but opposite :) I haven\u0027t made this a docstring because it\u0027s not part of the public interface. I don\u0027t know how consistent I\u0027ve been with this, tbh.","commit_id":"32be7a4a028d19a9fe929db20b6eb637f597175c"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"6dd6f09856e92dcd8adf20dc9ba4689c5799a0b2","unresolved":false,"context_lines":[{"line_number":288,"context_line":""},{"line_number":289,"context_line":"def mock_backend(disks\u003dNone, gotFiles\u003dNone, imported_files\u003dNone,"},{"line_number":290,"context_line":"                 exists\u003dNone):"},{"line_number":291,"context_line":"    # Return a mock which mocks imagebackend.Backend.backend to return an"},{"line_number":292,"context_line":"    # FakeImage class. Optionally collects various runtime data:"},{"line_number":293,"context_line":"    #"},{"line_number":294,"context_line":"    # disks: A dict of disk name -\u003e FakeImage objects"}],"source_content_type":"text/x-python","patch_set":7,"id":"3aaa91ec_f90c7594","line":291,"updated":"2016-07-01 10:07:43.000000000","message":"Can you use our docstring convention here ?","commit_id":"2bb68e34102590a060d8bc3dedbea46efb3bb32a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8c3653021ad63ce3c774f0121c684bfc3bccfae8","unresolved":false,"context_lines":[{"line_number":288,"context_line":""},{"line_number":289,"context_line":"def mock_backend(disks\u003dNone, gotFiles\u003dNone, imported_files\u003dNone,"},{"line_number":290,"context_line":"                 exists\u003dNone):"},{"line_number":291,"context_line":"    # Return a mock which mocks imagebackend.Backend.backend to return an"},{"line_number":292,"context_line":"    # FakeImage class. Optionally collects various runtime data:"},{"line_number":293,"context_line":"    #"},{"line_number":294,"context_line":"    # disks: A dict of disk name -\u003e FakeImage objects"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_1dd81253","line":291,"in_reply_to":"3aaa91ec_f90c7594","updated":"2016-07-04 12:03:52.000000000","message":"I guess I could. I deliberately don\u0027t use valid docstrings in tests because they result in confusing output from some test harnesses, but as this isn\u0027t actually a test I guess we can do it properly here.\n\nThis patch is *really* far down the patch series, though. I\u0027m not going to update just for this, but if there\u0027s anything else I\u0027ll update this.","commit_id":"2bb68e34102590a060d8bc3dedbea46efb3bb32a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"5a809ab37560ac23e3c5027ee1a37d0d4309a2dc","unresolved":false,"context_lines":[{"line_number":388,"context_line":""},{"line_number":389,"context_line":""},{"line_number":390,"context_line":"@contextlib.contextmanager"},{"line_number":391,"context_line":"def mock_backend(gotFiles\u003dNone, imported_files\u003dNone, exists\u003dNone):"},{"line_number":392,"context_line":"    \"\"\"Return a mock which mocks imagebackend.Backend.backend to return an"},{"line_number":393,"context_line":"    FakeImage class. Optionally collects various runtime data."},{"line_number":394,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_09e0b4f9","line":391,"updated":"2016-08-03 19:17:14.000000000","message":"Please rename gotFiles to got_files to be consistent with naming policy and the other variables here.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"0779271b89ec5d2d023a733dccb6fedb4015311a","unresolved":false,"context_lines":[{"line_number":388,"context_line":""},{"line_number":389,"context_line":""},{"line_number":390,"context_line":"@contextlib.contextmanager"},{"line_number":391,"context_line":"def mock_backend(gotFiles\u003dNone, imported_files\u003dNone, exists\u003dNone):"},{"line_number":392,"context_line":"    \"\"\"Return a mock which mocks imagebackend.Backend.backend to return an"},{"line_number":393,"context_line":"    FakeImage class. Optionally collects various runtime data."},{"line_number":394,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_f6117353","line":391,"updated":"2016-08-04 23:12:43.000000000","message":"ack on gotFiles and renaming it later, no prob and understood.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"54931e2d777e0c2bde6f8518297113cd15b5b1f2","unresolved":false,"context_lines":[{"line_number":388,"context_line":""},{"line_number":389,"context_line":""},{"line_number":390,"context_line":"@contextlib.contextmanager"},{"line_number":391,"context_line":"def mock_backend(gotFiles\u003dNone, imported_files\u003dNone, exists\u003dNone):"},{"line_number":392,"context_line":"    \"\"\"Return a mock which mocks imagebackend.Backend.backend to return an"},{"line_number":393,"context_line":"    FakeImage class. Optionally collects various runtime data."},{"line_number":394,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_6437bcb0","line":391,"in_reply_to":"bacf61ea_09e0b4f9","updated":"2016-08-04 10:17:12.000000000","message":"gotFiles is for compatibility with existing tests which use it. I get rid of it later in the series as I rewrite more tests. Renaming it would be counter-productive, because it would add the noise I\u0027m trying to avoid.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"5a809ab37560ac23e3c5027ee1a37d0d4309a2dc","unresolved":false,"context_lines":[{"line_number":430,"context_line":"        return DiskMocks("},{"line_number":431,"context_line":"            cache\u003dmock.Mock(side_effect\u003dfake_cache),"},{"line_number":432,"context_line":"            import_file\u003dmock.Mock(side_effect\u003dfake_import_file),"},{"line_number":433,"context_line":"            **default_mocks)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":"    mocks \u003d defaultdict(DiskMocks_factory)"},{"line_number":436,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_69e7b012","line":433,"updated":"2016-08-03 19:17:14.000000000","message":"Classes like this really just indicate to me that the tests in this file are not unit tests at all but rather functional tests masquerading as unit tests. I feel that this kind of factory mockout class just makes the code *more* obscure and the tests essentially unreadable even compared to the hardly-readable current state of the unit tests.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fe2d3d3dd0d8f2b87bade895ba941b74eafb9577","unresolved":false,"context_lines":[{"line_number":430,"context_line":"        return DiskMocks("},{"line_number":431,"context_line":"            cache\u003dmock.Mock(side_effect\u003dfake_cache),"},{"line_number":432,"context_line":"            import_file\u003dmock.Mock(side_effect\u003dfake_import_file),"},{"line_number":433,"context_line":"            **default_mocks)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":"    mocks \u003d defaultdict(DiskMocks_factory)"},{"line_number":436,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_82a72b0a","line":433,"in_reply_to":"bacf61ea_45a1cfa8","updated":"2016-08-04 15:06:44.000000000","message":"So, I also don\u0027t like the mechanics of this thing. It\u0027s a lot of complex stuff that defines classes, then mocks some of their things, all inside a context manager. I get what it\u0027s trying to do and why, but it\u0027s still pretty hairy, IMHO. It feels a lot like create_autospec() in mock. It\u0027d be nice if we could leverage more of that, but the fact that we have to subclass things like Image might make that hard, I dunno.\n\nI also agree with jay\u0027s assessment of the lack of unit-ness to these tests, but I think that\u0027s mostly pre-existing behavior. This mock-the-world approach makes it a little worse because we end up with some things mocked for all tests that don\u0027t need them to be, which is less desirable. The number of tests below that define their own fake Image class with most of the things stubbed out is a good demonstration I think. Many off them fully stub out the Backend and Image classes, even if they don\u0027t override them.\n\nI think maybe we should take what Matt has here (functionality, not mechanics) and move that into a nice tight fixture. We can test that fixture to make sure it\u0027s legit, and then tests here can use it just like we use fakelibvirt in other tests. It can give us a globalish view of all the things that have been done against all image/disk objects (which is Matt\u0027s goal), just like many of our other fixtures. I think he\u0027s basically defined a fixture here, which we like elsewhere, but it doesn\u0027t fit the pattern in its current form.\n\nAfter that, I think it would be excellent to move the more functional tests out of here and into the functional group for better separation. However, don\u0027t want to refactor them and move them at the same time, so we either have to move them and then refactor (which is much less desirable, IMHO) or fix them up here and then move them. Given the massive stack of patches to follow this, I think we probably have to just deal with these being here for a while until we\u0027re done with those, and then move the resulting cleaner tests to the appropriate groups as necessary.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"54931e2d777e0c2bde6f8518297113cd15b5b1f2","unresolved":false,"context_lines":[{"line_number":430,"context_line":"        return DiskMocks("},{"line_number":431,"context_line":"            cache\u003dmock.Mock(side_effect\u003dfake_cache),"},{"line_number":432,"context_line":"            import_file\u003dmock.Mock(side_effect\u003dfake_import_file),"},{"line_number":433,"context_line":"            **default_mocks)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":"    mocks \u003d defaultdict(DiskMocks_factory)"},{"line_number":436,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_45a1cfa8","line":433,"in_reply_to":"bacf61ea_69e7b012","updated":"2016-08-04 10:17:12.000000000","message":"Couple of things. Firstly please ignore the fake functions because I\u0027m not actually introducing them here. They\u0027re existing fakes for compatibility with existing tests that I\u0027m not fully rewriting in this patch, e.g. old line 10533 in this file. fake_cache goes away later in the series as I rewrite the last test which uses gotFiles. I haven\u0027t yet killed imported_files, but it\u0027s on the todo list. No new code uses gotFiles or imported_files, just assertions on mocks.\n\nSecondly, please note that this is about the 100th iteration of this code :) I\u0027ve been all the way to the end of this test series, and as I\u0027ve found the need for changes in the design I\u0027ve pulled them back here to avoid refactoring it during the series. Similarly, note that a bunch of bugs in tests uncovered by these changes were fixed earlier in the series and already merged.\n\nThis is not the test code; this is test library code. Any complexity it has is to avoid complexity in tests. For example, that\u0027s why this class goes to lengths to hide the fact that multiple Image objects may be instantiated for the same disk. If we didn\u0027t do that here, every test which asserted calls on Image objects would have to do it itself.\n\nI hadn\u0027t given it much thought until now, but it may even be possible in a future patch to do away with the FakeImage class below. However we can\u0027t do that here because validation of the refactor by unit tests relies on the fact that we continue to assert unchanged calls to cache() even after we replace them all in change Ie5d47f55. This requires executing the compatibility layer introduced in that patch. We deliberately don\u0027t update the tests until the next patch, change Ifc274924 (which is also where fake_cache goes away).\n\nFrom a design POV, this allows us to validate changes to irrelevant (and bad) interfaces internal to the driver module by mocking the whole imagebackend module. From a practical POV, it allows us to write much more expressive tests without faking the imagebackend module. It\u0027s also very far from irrelevant to note that this series uses this method so extensively that if I had to make any substantial design change to it I\u0027d have to rewrite about half of it. A -1 here is essentially a NAK on the whole series. If that\u0027s what\u0027s required then by all means, but please don\u0027t decide to do that lightly.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"0779271b89ec5d2d023a733dccb6fedb4015311a","unresolved":false,"context_lines":[{"line_number":10370,"context_line":"        # We should have imported \u0027disk.config\u0027"},{"line_number":10371,"context_line":"        config_disk \u003d backend.disks[\u0027disk.config\u0027]"},{"line_number":10372,"context_line":"        config_disk.import_file.assert_called_once_with(instance, mock.ANY,"},{"line_number":10373,"context_line":"                                                        \u0027disk.config\u0027)"},{"line_number":10374,"context_line":""},{"line_number":10375,"context_line":"    def test_spawn_without_image_meta(self):"},{"line_number":10376,"context_line":"        def fake_none(*args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_561b1f73","line":10373,"updated":"2016-08-04 23:12:43.000000000","message":"Here\u0027s an example of what I\u0027m talking about. The original version of test_spawn_with_config_drive() was actually testing the *correct* surface area for the unit test: that libvirt.driver.Driver._create_configdrive() was called with expected parameters when driver.spawn() was called with a particular instance request. The surface area of the original test code was the correct surface area: mocks of all the first-level subcalls from spawn were done and the call signature to _create_configdrive() was asserted, which was the point of the original unit test.\n\nThis new test *expands* the surface area to now go into the _create_configdrive() subcall and assert a bunch of stuff in *that* method, which makes the above unit test *less* readable than it was originally and improperly expanding the surface area of tested code to more than a single unit of code.\n\nThere should be separate unit tests for the code *inside* _create_configdrive() where assertions like the above would better fit.\n\nAnyway, that\u0027s what I\u0027m referring to when I say that unit test surface area is being expanded and abused... I\u0027m not going to hold this patch series up, but this is the kind of thing that I actually believe is making the unit tests less readable.","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"0779271b89ec5d2d023a733dccb6fedb4015311a","unresolved":false,"context_lines":[{"line_number":10393,"context_line":"        with mock_backend() as backend:"},{"line_number":10394,"context_line":"            drvr.spawn(self.context, instance, image_meta, [], None)"},{"line_number":10395,"context_line":""},{"line_number":10396,"context_line":"        # We should have created a root disk and an ephemeral disk"},{"line_number":10397,"context_line":"        self.assertEqual([\u0027disk\u0027, \u0027disk.local\u0027],"},{"line_number":10398,"context_line":"                         sorted(backend.created_disks.keys()))"},{"line_number":10399,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"bacf61ea_36182b75","line":10396,"updated":"2016-08-04 23:12:43.000000000","message":"Here\u0027s another example. These kinds of assertions belong in unit tests of the Driver._create_image() method, *not* unit tests of the Driver.spawn() method, which is what this particular unit test is for...","commit_id":"1b6b2f444ad04915e02ca2f6ff68614507e09586"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":10214,"context_line":"            context, xml, instance, network_info, disk_info,"},{"line_number":10215,"context_line":"            block_device_info\u003dNone, power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":10216,"context_line":"            vifs_already_plugged\u003dFalse, post_xml_callback\u003dNone"},{"line_number":10217,"context_line":"        ):"},{"line_number":10218,"context_line":"            # Config drive will only be called by post_xml_callback,"},{"line_number":10219,"context_line":"            # so we need to execute it"},{"line_number":10220,"context_line":"            with test.nested("}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_12ee4d22","line":10217,"updated":"2016-08-15 14:44:28.000000000","message":"This doesn\u0027t seem like the proper place for this. Can you put it on the line above?","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":10214,"context_line":"            context, xml, instance, network_info, disk_info,"},{"line_number":10215,"context_line":"            block_device_info\u003dNone, power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":10216,"context_line":"            vifs_already_plugged\u003dFalse, post_xml_callback\u003dNone"},{"line_number":10217,"context_line":"        ):"},{"line_number":10218,"context_line":"            # Config drive will only be called by post_xml_callback,"},{"line_number":10219,"context_line":"            # so we need to execute it"},{"line_number":10220,"context_line":"            with test.nested("}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_3ab7db3a","line":10217,"in_reply_to":"3ac371cc_12ee4d22","updated":"2016-08-15 16:15:46.000000000","message":"Sure, but eww, really? If you put these on the line above you\u0027ve got no strong visual distinction between the 2 nesting levels.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9eb6286ddf17583f2506dc22ad0b811289497c37","unresolved":false,"context_lines":[{"line_number":10214,"context_line":"            context, xml, instance, network_info, disk_info,"},{"line_number":10215,"context_line":"            block_device_info\u003dNone, power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":10216,"context_line":"            vifs_already_plugged\u003dFalse, post_xml_callback\u003dNone"},{"line_number":10217,"context_line":"        ):"},{"line_number":10218,"context_line":"            # Config drive will only be called by post_xml_callback,"},{"line_number":10219,"context_line":"            # so we need to execute it"},{"line_number":10220,"context_line":"            with test.nested("}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_bd40e557","line":10217,"in_reply_to":"3ac371cc_3ab7db3a","updated":"2016-08-15 16:37:58.000000000","message":"Yes, really. Indent the parameter lines to provide the distinction. That\u0027s what we do everywhere else. I don\u0027t know that I\u0027ve seen any of this pattern elsewhere in nova. It\u0027s also visually jarring to me to look like it\u0027s indenting, back, indented again.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":10221,"context_line":"                mock.patch.object(drvr, \u0027_build_device_metadata\u0027,"},{"line_number":10222,"context_line":"                                  return_value\u003dNone),"},{"line_number":10223,"context_line":"                mock.patch.object(instance_metadata, \u0027InstanceMetadata\u0027)"},{"line_number":10224,"context_line":"            ):"},{"line_number":10225,"context_line":"                post_xml_callback()"},{"line_number":10226,"context_line":""},{"line_number":10227,"context_line":"        fake_backend \u003d self.useFixture("}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_123c8d92","line":10224,"updated":"2016-08-15 14:44:28.000000000","message":"This one too. Also, this seems like crazy deep nesting, mocking things inside a mocked function, etc. Can\u0027t you just stack these as decorators on the outer test function to get them out of the way?","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":10221,"context_line":"                mock.patch.object(drvr, \u0027_build_device_metadata\u0027,"},{"line_number":10222,"context_line":"                                  return_value\u003dNone),"},{"line_number":10223,"context_line":"                mock.patch.object(instance_metadata, \u0027InstanceMetadata\u0027)"},{"line_number":10224,"context_line":"            ):"},{"line_number":10225,"context_line":"                post_xml_callback()"},{"line_number":10226,"context_line":""},{"line_number":10227,"context_line":"        fake_backend \u003d self.useFixture("}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_9a052fc4","line":10224,"in_reply_to":"3ac371cc_123c8d92","updated":"2016-08-15 16:15:46.000000000","message":"Sounds cleaner.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":10231,"context_line":"            mock.patch.object(drvr, \u0027_get_guest_xml\u0027),"},{"line_number":10232,"context_line":"            mock.patch.object(drvr, \u0027_create_domain_and_network\u0027,"},{"line_number":10233,"context_line":"                              fake_create_domain_and_network),"},{"line_number":10234,"context_line":"            mock.patch.object(drvr, \u0027get_info\u0027, return_value \u003d instance_info)"},{"line_number":10235,"context_line":"        ):"},{"line_number":10236,"context_line":"            drvr.spawn(self.context, instance,"},{"line_number":10237,"context_line":"                       image_meta, [], None)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_b2f939cb","line":10234,"range":{"start_line":10234,"start_character":60,"end_line":10234,"end_character":63},"updated":"2016-08-15 14:44:28.000000000","message":"Shouldn\u0027t be spaces around this.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":10231,"context_line":"            mock.patch.object(drvr, \u0027_get_guest_xml\u0027),"},{"line_number":10232,"context_line":"            mock.patch.object(drvr, \u0027_create_domain_and_network\u0027,"},{"line_number":10233,"context_line":"                              fake_create_domain_and_network),"},{"line_number":10234,"context_line":"            mock.patch.object(drvr, \u0027get_info\u0027, return_value \u003d instance_info)"},{"line_number":10235,"context_line":"        ):"},{"line_number":10236,"context_line":"            drvr.spawn(self.context, instance,"},{"line_number":10237,"context_line":"                       image_meta, [], None)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_5ac9f777","line":10234,"range":{"start_line":10234,"start_character":60,"end_line":10234,"end_character":63},"in_reply_to":"3ac371cc_b2f939cb","updated":"2016-08-15 16:15:46.000000000","message":"pep8 failed me!\n\nSure.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":10232,"context_line":"            mock.patch.object(drvr, \u0027_create_domain_and_network\u0027,"},{"line_number":10233,"context_line":"                              fake_create_domain_and_network),"},{"line_number":10234,"context_line":"            mock.patch.object(drvr, \u0027get_info\u0027, return_value \u003d instance_info)"},{"line_number":10235,"context_line":"        ):"},{"line_number":10236,"context_line":"            drvr.spawn(self.context, instance,"},{"line_number":10237,"context_line":"                       image_meta, [], None)"},{"line_number":10238,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_926fdd91","line":10235,"updated":"2016-08-15 14:44:28.000000000","message":"This paren too, and same argument for the mocks.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d17a3ffc49b6043d97f8cdfea9502460a8ebadc1","unresolved":false,"context_lines":[{"line_number":10232,"context_line":"            mock.patch.object(drvr, \u0027_create_domain_and_network\u0027,"},{"line_number":10233,"context_line":"                              fake_create_domain_and_network),"},{"line_number":10234,"context_line":"            mock.patch.object(drvr, \u0027get_info\u0027, return_value \u003d instance_info)"},{"line_number":10235,"context_line":"        ):"},{"line_number":10236,"context_line":"            drvr.spawn(self.context, instance,"},{"line_number":10237,"context_line":"                       image_meta, [], None)"},{"line_number":10238,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_5ac5773a","line":10235,"in_reply_to":"3ac371cc_926fdd91","updated":"2016-08-15 16:15:46.000000000","message":"Ok, but again, eww. I think this is much easier to read. I could double-indent the stuff above, I guess.","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5e0d8ed48c31e64f0d47470a7e29a5b737bf760c","unresolved":false,"context_lines":[{"line_number":10557,"context_line":"            mock.patch.object(driver, \u0027_create_domain_and_network\u0027),"},{"line_number":10558,"context_line":"            mock.patch.object(driver, \u0027get_info\u0027,"},{"line_number":10559,"context_line":"              return_value\u003d[hardware.InstanceInfo(state\u003dpower_state.RUNNING)])"},{"line_number":10560,"context_line":"        ):"},{"line_number":10561,"context_line":"            image_meta \u003d objects.ImageMeta.from_dict(self.test_image_meta)"},{"line_number":10562,"context_line":"            disk_info \u003d blockinfo.get_disk_info(CONF.libvirt.virt_type,"},{"line_number":10563,"context_line":"                                                instance,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3ac371cc_72dd0142","line":10560,"updated":"2016-08-15 14:44:28.000000000","message":"...","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d8a872a8d98cc435ebb8971161c7cc15696976b2","unresolved":false,"context_lines":[{"line_number":10593,"context_line":"            mock.patch.object(driver, \u0027_create_domain_and_network\u0027),"},{"line_number":10594,"context_line":"            mock.patch.object(driver, \u0027get_info\u0027,"},{"line_number":10595,"context_line":"              return_value\u003d[hardware.InstanceInfo(state\u003dpower_state.RUNNING)])"},{"line_number":10596,"context_line":"        ):"},{"line_number":10597,"context_line":"            image_meta \u003d objects.ImageMeta.from_dict(self.test_image_meta)"},{"line_number":10598,"context_line":"            disk_info \u003d blockinfo.get_disk_info(CONF.libvirt.virt_type,"},{"line_number":10599,"context_line":"                                                instance,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1ac06dbe_a42f9b3d","line":10596,"updated":"2016-08-23 16:22:33.000000000","message":"random whitespace damage :(","commit_id":"32be7a4a028d19a9fe929db20b6eb637f597175c"}]}