)]}'
{"tempest/api/compute/volumes/test_attach_volume.py":[{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"78cec28104aab73a7447eac9240debcb2dafdfe6","unresolved":false,"context_lines":[{"line_number":64,"context_line":"            self.volumes_client.wait_for_resource_deletion(self.volume[\u0027id\u0027])"},{"line_number":65,"context_line":"            self.volume \u003d None"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"    def _create_and_attach(self, shelve_server\u003dFalse, count_volumes\u003dFalse):"},{"line_number":68,"context_line":"        # Start a server and wait for it to become ready"},{"line_number":69,"context_line":"        self.admin_pass \u003d self.image_ssh_password"},{"line_number":70,"context_line":"        self.server \u003d self.create_test_server("}],"source_content_type":"text/x-python","patch_set":4,"id":"dada55a8_33d533b9","line":67,"updated":"2016-07-19 08:43:49.000000000","message":"This method does too much now. You need to split it. Also you are duplicating some code (l82-l91 are the same than l210-l220).","commit_id":"519d5253ba4432b8d1851782534f0ac64139357b"},{"author":{"_account_id":13454,"name":"Alexander Gubanov","email":"ogubanov@mirantis.com","username":"hop"},"change_message_id":"7303646109264858cddaa17ffe127c338bb1e413","unresolved":false,"context_lines":[{"line_number":87,"context_line":"                server\u003dself.server,"},{"line_number":88,"context_line":"                servers_client\u003dself.servers_client)"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"            command \u003d \u0027grep -E [vs]d.$ /proc/partitions | wc -l\u0027"},{"line_number":91,"context_line":"            volumes \u003d int(linux_client.exec_command(command).strip())"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        # Create a volume and wait for it to become ready"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_7e44b91a","line":90,"range":{"start_line":90,"start_character":45,"end_line":90,"end_character":55},"updated":"2016-07-14 10:13:26.000000000","message":"2 notes:\n- grep has \"-c\" option for counting lines, so you can use it instead of \"wc -l\"\n- it would be nice to wrap regex with double quotes","commit_id":"519d5253ba4432b8d1851782534f0ac64139357b"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"31ed530741f97511d7f1467786d00bdcfa76f729","unresolved":false,"context_lines":[{"line_number":64,"context_line":"            self.volumes_client.wait_for_resource_deletion(self.volume[\u0027id\u0027])"},{"line_number":65,"context_line":"            self.volume \u003d None"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"    def _count_volumes(self, server):"},{"line_number":68,"context_line":"        # Count number of volumes on an instance"},{"line_number":69,"context_line":"        linux_client \u003d remote_client.RemoteClient("},{"line_number":70,"context_line":"            self.get_server_ip(self.server),"}],"source_content_type":"text/x-python","patch_set":10,"id":"9ad45d7e_eb434011","line":67,"updated":"2016-08-11 00:58:49.000000000","message":"This is only ever really used in the child class right? This should live there then","commit_id":"8d60148886934e9a68fad771c92724cd8b306d08"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"31ed530741f97511d7f1467786d00bdcfa76f729","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        # Count number of volumes before volume attachment"},{"line_number":93,"context_line":"        volumes \u003d 0"},{"line_number":94,"context_line":"        if count_volumes and CONF.validation.run_validation:"},{"line_number":95,"context_line":"            volumes \u003d self._count_volumes(self.server)"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        # Create a volume and wait for it to become ready"},{"line_number":98,"context_line":"        self.volume \u003d self.volumes_client.create_volume("}],"source_content_type":"text/x-python","patch_set":10,"id":"9ad45d7e_4b4d2c3d","line":95,"updated":"2016-08-11 00:58:49.000000000","message":"So you added the helper function to get the number of volumes to the parent class so you could reuse this entire function but only in the clid class. I think this violates the structural split we want to have. Granted the fundamental structure of the tests here is very weird and relies on instance variables to pass data around\n\nI think ideally what we really want is to have this function split up and done in each test individually instead. This not only makes the test code much cleaner but it also means you don\u0027t have to artificially inject a bunch of logic that doesn\u0027t apply here. Instead you can localize the volume counting to the places it\u0027s actually used.\n\nSo instead of one giant function that tries to do all the logic for all of the tests, we should have a bunch of smaller functions that do 1 thing. (or you can just just call the clients directly in the test methods)","commit_id":"8d60148886934e9a68fad771c92724cd8b306d08"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"b56cbbc8699a52d17809a411b118403eca0013fb","unresolved":false,"context_lines":[{"line_number":218,"context_line":"                                       \u0027ACTIVE\u0027)"},{"line_number":219,"context_line":"        if CONF.validation.run_validation:"},{"line_number":220,"context_line":"            nb_volumes \u003d self._count_volumes(self.server)"},{"line_number":221,"context_line":"            self.assertEqual(number_of_volume, nb_volumes)"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    @test.idempotent_id(\u002713a940b6-3474-4c3c-b03f-29b89112bfee\u0027)"},{"line_number":224,"context_line":"    @testtools.skipUnless(CONF.compute_feature_enabled.shelve,"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa7ab95a_e13684e6","line":221,"updated":"2016-08-31 11:48:00.000000000","message":"One of the 2 variables here must be renamed. As a code reviewer, I wonder how \u0027number of volumes\u0027 could be different than \u0027number of volumes\u0027.","commit_id":"04afab2b7d15607602c58716ee01e90f95fdf937"},{"author":{"_account_id":16437,"name":"Anna Babich","email":"anna.v.babich@gmail.com","username":"ababich"},"change_message_id":"b1a8d236cf49f0e5f72bb059c5dfa6548cc80056","unresolved":false,"context_lines":[{"line_number":218,"context_line":"                                       \u0027ACTIVE\u0027)"},{"line_number":219,"context_line":"        if CONF.validation.run_validation:"},{"line_number":220,"context_line":"            nb_volumes \u003d self._count_volumes(self.server)"},{"line_number":221,"context_line":"            self.assertEqual(number_of_volume, nb_volumes)"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    @test.idempotent_id(\u002713a940b6-3474-4c3c-b03f-29b89112bfee\u0027)"},{"line_number":224,"context_line":"    @testtools.skipUnless(CONF.compute_feature_enabled.shelve,"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa7ab95a_44173e54","line":221,"in_reply_to":"fa7ab95a_e13684e6","updated":"2016-08-31 12:14:11.000000000","message":"Done","commit_id":"04afab2b7d15607602c58716ee01e90f95fdf937"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"b56cbbc8699a52d17809a411b118403eca0013fb","unresolved":false,"context_lines":[{"line_number":259,"context_line":"        self._detach(self.server[\u0027id\u0027], self.volume[\u0027id\u0027])"},{"line_number":260,"context_line":"        self.attachment \u003d None"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        # Unshelve the instance and check that there is no volume attached"},{"line_number":263,"context_line":"        self._unshelve_server_and_check_volumes(num_vol)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa7ab95a_41cf30b3","line":262,"updated":"2016-08-31 11:48:00.000000000","message":"\u0027no volume attached\u0027 ? If num_vol \u003d\u003d 1, then the code actually checks that there\u0027s one volume attached. So I find the code comment misleading. What about \u0027Unshelve the instance and check that we have the expected number of volume(s)\u0027 ?","commit_id":"04afab2b7d15607602c58716ee01e90f95fdf937"},{"author":{"_account_id":16437,"name":"Anna Babich","email":"anna.v.babich@gmail.com","username":"ababich"},"change_message_id":"b1a8d236cf49f0e5f72bb059c5dfa6548cc80056","unresolved":false,"context_lines":[{"line_number":259,"context_line":"        self._detach(self.server[\u0027id\u0027], self.volume[\u0027id\u0027])"},{"line_number":260,"context_line":"        self.attachment \u003d None"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        # Unshelve the instance and check that there is no volume attached"},{"line_number":263,"context_line":"        self._unshelve_server_and_check_volumes(num_vol)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa7ab95a_84114660","line":262,"in_reply_to":"fa7ab95a_41cf30b3","updated":"2016-08-31 12:14:11.000000000","message":"Done","commit_id":"04afab2b7d15607602c58716ee01e90f95fdf937"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"1f9db373a834b227b2f26386cd04075155956657","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"    def _create_server(self):"},{"line_number":68,"context_line":"        # Start a server and wait for it to become ready"},{"line_number":69,"context_line":"        self.admin_pass \u003d self.image_ssh_password"},{"line_number":70,"context_line":"        self.server \u003d self.create_test_server("},{"line_number":71,"context_line":"            validatable\u003dTrue,"},{"line_number":72,"context_line":"            wait_until\u003d\u0027ACTIVE\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa7ab95a_92feda03","line":69,"updated":"2016-08-31 15:16:43.000000000","message":"Why do we need to set admin_pass here? can\u0027t we just access self.image_ssh_password directly?","commit_id":"a755229becdba85557860dccaf93e6da5fb70c79"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"1f9db373a834b227b2f26386cd04075155956657","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    def _create_server(self):"},{"line_number":68,"context_line":"        # Start a server and wait for it to become ready"},{"line_number":69,"context_line":"        self.admin_pass \u003d self.image_ssh_password"},{"line_number":70,"context_line":"        self.server \u003d self.create_test_server("},{"line_number":71,"context_line":"            validatable\u003dTrue,"},{"line_number":72,"context_line":"            wait_until\u003d\u0027ACTIVE\u0027,"},{"line_number":73,"context_line":"            adminPass\u003dself.admin_pass)"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa7ab95a_b203de0c","line":70,"updated":"2016-08-31 15:16:43.000000000","message":"So you\u0027re still using an instance variable here which is different that how it\u0027s being used in the tests. This should be:\n\n   server \u003d self.create_server(...\n\n   ...\n\n   return server","commit_id":"a755229becdba85557860dccaf93e6da5fb70c79"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"1f9db373a834b227b2f26386cd04075155956657","unresolved":false,"context_lines":[{"line_number":98,"context_line":"    def test_attach_detach_volume(self):"},{"line_number":99,"context_line":"        # Stop and Start a server with an attached volume, ensuring that"},{"line_number":100,"context_line":"        # the volume remains attached."},{"line_number":101,"context_line":"        server \u003d self._create_server()"},{"line_number":102,"context_line":"        self._create_and_attach_volume(server)"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"        self.servers_client.stop_server(self.server[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa7ab95a_d208622b","line":101,"updated":"2016-08-31 15:16:43.000000000","message":"So this is a bit weird, server here is none and you\u0027re passing the actual server around as an instance variable self.server inside these helper functions. You\u0027ve got all the pieces to properly scope the variable here though","commit_id":"a755229becdba85557860dccaf93e6da5fb70c79"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"3448308af78dec789ed3b5c1d475683168b2d489","unresolved":false,"context_lines":[{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    def _create_and_attach_volume(self, server):"},{"line_number":80,"context_line":"        # Create a volume and wait for it to become ready"},{"line_number":81,"context_line":"        self.volume \u003d self.volumes_client.create_volume("},{"line_number":82,"context_line":"            size\u003dCONF.volume.volume_size, display_name\u003d\u0027test\u0027)[\u0027volume\u0027]"},{"line_number":83,"context_line":"        self.addCleanup(self._delete_volume)"},{"line_number":84,"context_line":"        waiters.wait_for_volume_status(self.volumes_client,"}],"source_content_type":"text/x-python","patch_set":15,"id":"9a89bdaa_35521fde","line":81,"updated":"2016-09-06 14:36:09.000000000","message":"A future cleanup if you\u0027re willing would be to remove the self.volume here and just use volume here and then return volume. Having an instance variable for volume doesn\u0027t buy us anything here, but we can do this in a follow up.","commit_id":"bbef276a826bd59999334f7760889a9032f561fd"}]}