)]}'
{"tempest/api/volume/test_volumes_snapshots.py":[{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"bbea55f0aea36f50c559b1fd2824b68b8e493c50","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        for snap in xrange(2):"},{"line_number":64,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":65,"context_line":"        params \u003d {\u0027limit\u0027: 1}"},{"line_number":66,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":67,"context_line":"        if with_detail:"},{"line_number":68,"context_line":"            fetched_snp_list \u003d self.snapshots_client.list_snapshots("},{"line_number":69,"context_line":"                detail\u003dTrue, **params)[\u0027snapshots\u0027]"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba0121b8_fce341a2","line":66,"updated":"2016-03-30 16:13:41.000000000","message":"This would look better:\n\n  fetched_snp_list \u003d self.snapshots_client.list_snapshots(\n     detail\u003dwith_detail, **params)[\u0027snapshots\u0027]","commit_id":"da5416784b1725f1f33881158d5731110803228f"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"bbea55f0aea36f50c559b1fd2824b68b8e493c50","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            fetched_snp_list \u003d self.snapshots_client.list_snapshots("},{"line_number":72,"context_line":"                **params)[\u0027snapshots\u0027]"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        self.assertEqual(params[\u0027limit\u0027],"},{"line_number":75,"context_line":"                         len(fetched_snp_list),"},{"line_number":76,"context_line":"                         \"Failed to list snapshot details by limit\")"},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba0121b8_974ade24","line":74,"updated":"2016-03-30 16:13:41.000000000","message":"You don\u0027t seem to assert specific things depending on the value of \"with_detail\", so why do you need this \"with_detail\" to begin with ?","commit_id":"da5416784b1725f1f33881158d5731110803228f"},{"author":{"_account_id":12024,"name":"Tong Liu","email":"tongl@vmware.com","username":"tong101"},"change_message_id":"69ee9c7164c9fc7a3d20d7ba35c58f380c660e0b","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            fetched_snp_list \u003d self.snapshots_client.list_snapshots("},{"line_number":72,"context_line":"                **params)[\u0027snapshots\u0027]"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        self.assertEqual(params[\u0027limit\u0027],"},{"line_number":75,"context_line":"                         len(fetched_snp_list),"},{"line_number":76,"context_line":"                         \"Failed to list snapshot details by limit\")"},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba0121b8_54ec4bba","line":74,"in_reply_to":"ba0121b8_974ade24","updated":"2016-04-01 17:44:49.000000000","message":"Agree. I don\u0027t think we need to begin with with_detail. This can be put in to the param dict in L65.\n\n params \u003d {\u0027detail\u0027: with_details, \u0027limit\u0027: 1}","commit_id":"da5416784b1725f1f33881158d5731110803228f"},{"author":{"_account_id":12024,"name":"Tong Liu","email":"tongl@vmware.com","username":"tong101"},"change_message_id":"c926de75ef52765e324be1a6ee914d5e8f128c8e","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        # Create two snapshots"},{"line_number":63,"context_line":"        for snap in xrange(2):"},{"line_number":64,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":65,"context_line":"        params \u003d {\u0027limit\u0027: 1}"},{"line_number":66,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":67,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":68,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_940a63d0","line":65,"updated":"2016-04-01 17:42:53.000000000","message":"It is more straightforward to put detail here and use **params later.\n\n params \u003d {\u0027detail\u0027: with_details, \u0027limit\u0027: 1}","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"c6f1107d4cb8d6c2db9e4dd6dc17552f42c7616f","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        # Create two snapshots"},{"line_number":63,"context_line":"        for snap in xrange(2):"},{"line_number":64,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":65,"context_line":"        params \u003d {\u0027limit\u0027: 1}"},{"line_number":66,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":67,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":68,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_27786ff8","line":65,"in_reply_to":"ba0121b8_940a63d0","updated":"2016-04-03 10:49:27.000000000","message":"Thanks for your comment, it is little bit confuse.\nI think detail should be separated from **params, like the api call:\n\ndef list_snapshots(self, detail\u003dFalse, params\u003dNone):\n        url \u003d \u0027os-snapshots\u0027\n\n        if detail:\n            url +\u003d \u0027/detail\u0027\n        if params:\n            url +\u003d \u0027?%s\u0027 % urllib.urlencode(params)","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":9152,"name":"Castulo J. Martinez","email":"castulo.martinez@intel.com","username":"cas"},"change_message_id":"d8f959de33ce55e8512e81a3347f6af389e63357","unresolved":false,"context_lines":[{"line_number":68,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":69,"context_line":"        self.assertEqual(params[\u0027limit\u0027],"},{"line_number":70,"context_line":"                         len(fetched_snap_list),"},{"line_number":71,"context_line":"                         \"Failed to list snapshot details by limit\")"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":74,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_9885755b","line":71,"range":{"start_line":71,"start_character":26,"end_line":71,"end_character":66},"updated":"2016-04-01 18:36:20.000000000","message":"Also, if you are going to include a message for when the assertion fails, I think you should add what was the expected value and the actual value to the message for it to be more helpful for the user.","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"c6f1107d4cb8d6c2db9e4dd6dc17552f42c7616f","unresolved":false,"context_lines":[{"line_number":68,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":69,"context_line":"        self.assertEqual(params[\u0027limit\u0027],"},{"line_number":70,"context_line":"                         len(fetched_snap_list),"},{"line_number":71,"context_line":"                         \"Failed to list snapshot details by limit\")"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":74,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_077d6be7","line":71,"range":{"start_line":71,"start_character":26,"end_line":71,"end_character":66},"in_reply_to":"ba0121b8_9885755b","updated":"2016-04-03 10:49:27.000000000","message":"Thanks for your comment, I will update the error message","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":9152,"name":"Castulo J. Martinez","email":"castulo.martinez@intel.com","username":"cas"},"change_message_id":"50fb68c7648567148827696faa419b83fd0fc93f","unresolved":false,"context_lines":[{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":195,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":196,"context_line":"        self._list_snapshots_by_param_limit(with_detail\u003dTrue)"},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":199,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_b4dfa70b","line":196,"range":{"start_line":196,"start_character":8,"end_line":196,"end_character":61},"updated":"2016-04-01 18:25:31.000000000","message":"I don\u0027t see the point of doing this test in a separate method. I don\u0027t think the code on that function can be reused much anyway.","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"c6f1107d4cb8d6c2db9e4dd6dc17552f42c7616f","unresolved":false,"context_lines":[{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":195,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":196,"context_line":"        self._list_snapshots_by_param_limit(with_detail\u003dTrue)"},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":199,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba0121b8_478763d7","line":196,"range":{"start_line":196,"start_character":8,"end_line":196,"end_character":61},"in_reply_to":"ba0121b8_b4dfa70b","updated":"2016-04-03 10:49:27.000000000","message":"The method can be used for two cases, with detail and without detail","commit_id":"5fd0c9f9e95c1f84008ad534150a00cc6b21ecdd"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"0678fc6e9efa6d59898ce5d39c593db6cceb855f","unresolved":false,"context_lines":[{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, with_detail\u003dFalse):"},{"line_number":62,"context_line":"        # Create two snapshots"},{"line_number":63,"context_line":"        for snap in xrange(2):"},{"line_number":64,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":65,"context_line":"        params \u003d {\u0027limit\u0027: 1}"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba0121b8_ca9b7a71","line":62,"range":{"start_line":62,"start_character":21,"end_line":62,"end_character":30},"updated":"2016-04-03 11:25:11.000000000","message":"Hi Liron ,\nIMO the _list_snapshots_ function should not create snapshots \nit should only do the snapshot list logic with assert.\nYou can add snapshots in the testcase body  \n\nBenny","commit_id":"32fd700546dcd881c798365b7c66570717e49ca2"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"be37bea2270f0cb867437681db83a2939f99437d","unresolved":false,"context_lines":[{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        failed_msg \u003d \"Failed to list snapshots {0} by limit;\" \\"},{"line_number":68,"context_line":"                     \" expected value: {1}, observed value: {2}\"\\"},{"line_number":69,"context_line":"            .format(\u0027with details\u0027 if with_detail else \u0027\u0027,"},{"line_number":70,"context_line":"                    params[\u0027limit\u0027], len(fetched_snap_list))"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a061dce_7a4e95c2","line":67,"updated":"2016-04-04 08:16:02.000000000","message":"We don\u0027t need this message. assertEqual will already print a similar message in case of failure.\n\nJust delete your failed_msg here and remove the 3rd parameter of assertEqual(...)","commit_id":"37a8862784bca7b7151d060558920dce1ae355ca"},{"author":{"_account_id":4727,"name":"Itzik Brown","email":"itzikb@redhat.com","username":"itzikb1"},"change_message_id":"8693573858567d779caf56c402a5f74c510b5c63","unresolved":false,"context_lines":[{"line_number":194,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":195,"context_line":""},{"line_number":196,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":197,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":198,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":199,"context_line":"        for snap in xrange(2):"},{"line_number":200,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba0121b8_c547eb49","line":197,"updated":"2016-04-03 12:45:07.000000000","message":"IMHO There is no need for two tests - one with detail and one without.","commit_id":"37a8862784bca7b7151d060558920dce1ae355ca"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"a94032c2171012b601e2208f12ec0e17b3286042","unresolved":false,"context_lines":[{"line_number":194,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":195,"context_line":""},{"line_number":196,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":197,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":198,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":199,"context_line":"        for snap in xrange(2):"},{"line_number":200,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a061dce_51e73412","line":197,"in_reply_to":"9a061dce_5ad8f11d","updated":"2016-04-04 09:50:42.000000000","message":"Thanks for your comment, I agree with you","commit_id":"37a8862784bca7b7151d060558920dce1ae355ca"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"be37bea2270f0cb867437681db83a2939f99437d","unresolved":false,"context_lines":[{"line_number":194,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":195,"context_line":""},{"line_number":196,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":197,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":198,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":199,"context_line":"        for snap in xrange(2):"},{"line_number":200,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a061dce_5ad8f11d","line":197,"in_reply_to":"ba0121b8_c547eb49","updated":"2016-04-04 08:16:02.000000000","message":"Agreed. Please don\u0027t test with \"detail\u003dTrue\". You don\u0027t do anything special in the test with the additional info you get with detail\u003dTrue. This is important as \u0027create_snapshot\u0027 is slow, we should limit the number of time we call it.","commit_id":"37a8862784bca7b7151d060558920dce1ae355ca"},{"author":{"_account_id":9152,"name":"Castulo J. Martinez","email":"castulo.martinez@intel.com","username":"cas"},"change_message_id":"2a1b76761b9cd65c4244324460df9d723802ad7e","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list),"},{"line_number":68,"context_line":"                         \"Failed to list snapshots by limit\")"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":71,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_7c231606","line":68,"range":{"start_line":68,"start_character":25,"end_line":68,"end_character":60},"updated":"2016-04-04 16:37:20.000000000","message":"The problem with using this assertion message is that if the test fails you are just gonna get this message:\nAssertionError: Failed to list snapshots by limit\nWhich doesn\u0027t provide extra value. I think it is better as Jordan Pittier recommended to just remove the whole message and leave:\nself.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list))\nso if the test fails it would show something like this:\nAssertionError: 1 !\u003d 2","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"344844490450561946b8a8a0a1a473baa1de0b40","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list),"},{"line_number":68,"context_line":"                         \"Failed to list snapshots by limit\")"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":71,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_cb1c596c","line":68,"range":{"start_line":68,"start_character":25,"end_line":68,"end_character":60},"in_reply_to":"9a061dce_09ba5e6e","updated":"2016-04-07 16:22:56.000000000","message":"This message doesn\u0027t bring any value. It basically says \"oups something went wrong\". That doesn\u0027t help and its obvious because the test failed.\n\n(sorry for being a bit rude, I am french).","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"afba949dd213b2b100a7b07ac70af0a6530b96e8","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list),"},{"line_number":68,"context_line":"                         \"Failed to list snapshots by limit\")"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":71,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_9a889324","line":68,"range":{"start_line":68,"start_character":25,"end_line":68,"end_character":60},"in_reply_to":"9a061dce_7c231606","updated":"2016-04-05 05:48:35.000000000","message":"assertEqual allows us to add exstra information, related to failure. the message is not instead of the information that assertEqual raises.\nThis is the message when the test fails:  \ntesttools.matchers._impl.MismatchError: 2 !\u003d 1: Failed to list snapshots by limit\nlike you see from the following code matcher is responsible for that:\n\ndef assertEqual(self, expected, observed, message\u003d\u0027\u0027):\n-------\u003ematcher \u003d _FlippedEquals(expected)\n        self.assertThat(observed, matcher, message)\n\n\ndef __init__(self, expected):\n        self._expected \u003d expected\n\n    def match(self, other):\n        mismatch \u003d Equals(self._expected).match(other)\n        if not mismatch:\n            return None\n-----\u003e  return _BinaryMismatch(other, \u0027!\u003d\u0027, self._expected, False)","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":9152,"name":"Castulo J. Martinez","email":"castulo.martinez@intel.com","username":"cas"},"change_message_id":"2e84b7baaad7e4e2b7ed5a6ee358a6d6b2f55357","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list),"},{"line_number":68,"context_line":"                         \"Failed to list snapshots by limit\")"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":71,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_09ba5e6e","line":68,"range":{"start_line":68,"start_character":25,"end_line":68,"end_character":60},"in_reply_to":"9a061dce_9a889324","updated":"2016-04-05 16:35:36.000000000","message":"You are right, the mismatch error is included with the assertion message. You can disregard my last comment then.  Looks good.","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"7c9043302c9670a9689d2acf98654c2be8b56a98","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            detail\u003dwith_detail, **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list),"},{"line_number":68,"context_line":"                         \"Failed to list snapshots by limit\")"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"},{"line_number":71,"context_line":"    @test.services(\u0027compute\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_f801d23a","line":68,"range":{"start_line":68,"start_character":25,"end_line":68,"end_character":60},"in_reply_to":"9a061dce_cb1c596c","updated":"2016-04-10 06:22:12.000000000","message":"Thanks, it will be fixed.","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"344844490450561946b8a8a0a1a473baa1de0b40","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":192,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":193,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":194,"context_line":"        for snap in xrange(2):"},{"line_number":195,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_6b66eded","line":192,"updated":"2016-04-07 16:22:56.000000000","message":"with_detail ?","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"7c9043302c9670a9689d2acf98654c2be8b56a98","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":192,"context_line":"    def test_snapshots_list_with_detail_param_limit(self):"},{"line_number":193,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":194,"context_line":"        for snap in xrange(2):"},{"line_number":195,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_d8fe8e37","line":192,"in_reply_to":"9a061dce_6b66eded","updated":"2016-04-10 06:22:12.000000000","message":"Thanks, my mistake","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"344844490450561946b8a8a0a1a473baa1de0b40","unresolved":false,"context_lines":[{"line_number":193,"context_line":"        # Verify list snapshot details by limit param"},{"line_number":194,"context_line":"        for snap in xrange(2):"},{"line_number":195,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":196,"context_line":"        self._list_snapshots_by_param_limit(with_detail\u003dFalse, limit\u003d1)"},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":199,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a061dce_2ba7d50b","line":196,"updated":"2016-04-07 16:22:56.000000000","message":"Or without detail ?","commit_id":"86f434f7baf87e9d9e75c8c7cf0c397cdbfc4730"},{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"ee8d16c7242c1da56b73e7d47113c3d0485c32c9","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, limit\u003d1):"},{"line_number":62,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":63,"context_line":"        params \u003d {\u0027limit\u0027: limit}"},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a1b2928_6ae4ff3a","line":61,"range":{"start_line":61,"start_character":50,"end_line":61,"end_character":52},"updated":"2016-04-10 23:38:44.000000000","message":"This default value seems meaningless because the test purpose is for specifying limit as the method name.","commit_id":"f19496c9fe1a9af73e45cd5902362314fe9684ff"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"263c123577e1d3ae560e9f1c00c50dba62204bfd","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, limit\u003d1):"},{"line_number":62,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":63,"context_line":"        params \u003d {\u0027limit\u0027: limit}"},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a1b2928_dd787cfb","line":61,"range":{"start_line":61,"start_character":50,"end_line":61,"end_character":52},"in_reply_to":"7a1b2928_6ae4ff3a","updated":"2016-04-11 06:04:31.000000000","message":"Thanks for your comment, it\u0027ll be fixed","commit_id":"f19496c9fe1a9af73e45cd5902362314fe9684ff"},{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"ee8d16c7242c1da56b73e7d47113c3d0485c32c9","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        params \u003d {\u0027limit\u0027: limit}"},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list))"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a1b2928_aad45747","line":66,"range":{"start_line":66,"start_character":12,"end_line":66,"end_character":20},"updated":"2016-04-10 23:38:44.000000000","message":"We can set like\n\n limit\u003dlimit)[\u0027snapshots\u0027]\n\nfor simple code.","commit_id":"f19496c9fe1a9af73e45cd5902362314fe9684ff"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"263c123577e1d3ae560e9f1c00c50dba62204bfd","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        params \u003d {\u0027limit\u0027: limit}"},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":66,"context_line":"            **params)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        self.assertEqual(params[\u0027limit\u0027], len(fetched_snap_list))"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a1b2928_5d0c6c42","line":66,"range":{"start_line":66,"start_character":12,"end_line":66,"end_character":20},"in_reply_to":"7a1b2928_aad45747","updated":"2016-04-11 06:04:31.000000000","message":"As above","commit_id":"f19496c9fe1a9af73e45cd5902362314fe9684ff"},{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"ee8d16c7242c1da56b73e7d47113c3d0485c32c9","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        # Verify list snapshot by limit param"},{"line_number":193,"context_line":"        for snap in xrange(2):"},{"line_number":194,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":195,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":198,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a1b2928_0acaaba1","line":195,"range":{"start_line":195,"start_character":8,"end_line":195,"end_character":52},"updated":"2016-04-10 23:38:44.000000000","message":"Why don\u0027t you add limit\u003d2 case here?","commit_id":"f19496c9fe1a9af73e45cd5902362314fe9684ff"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"b30fd2fcf6e499efb18220d47a0303a3075f87f4","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, limit\u003dNone):"},{"line_number":62,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":63,"context_line":"        params \u003d {\u0027limit\u0027: limit}"},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a1b2928_e7b95dee","line":61,"range":{"start_line":61,"start_character":45,"end_line":61,"end_character":55},"updated":"2016-04-11 09:02:07.000000000","message":"may be just make it non-optional, because with optional None value L65 will return all elements and L67 fails.\n\nw.r.t. below comment this can be like-\n    def _list_snapshots_by_param_limit(self, limit, expected_elements\u003dNone):\n        \"\"\"list snapshots by limit param\"\"\"\n        # Get snapshots list using limit parameter\n        fetched_snap_list \u003d self.snapshots_client.list_snapshots(\n            limit\u003dlimit)[\u0027snapshots\u0027]\n        expected_elements \u003d expected_elements or limit\n        self.assertEqual(expected_elements, len(fetched_snap_list))","commit_id":"f11d5991e899ca4d61d4d2780d92562caab04f91"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"b30fd2fcf6e499efb18220d47a0303a3075f87f4","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        # Verify list snapshot by limit param"},{"line_number":193,"context_line":"        for snap in xrange(2):"},{"line_number":194,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":195,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":198,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a1b2928_b1be87fd","line":195,"range":{"start_line":195,"start_character":8,"end_line":195,"end_character":51},"updated":"2016-04-11 09:02:07.000000000","message":"I think you can add below cases also- \nlimit\u003d0 - nothing returned \nlimit\u003d10000  - returned same number of element as self.snapshots_client.list_snapshots()\n\nsomething like-\n\n    def test_snapshots_list_param_limit(self):\n        # Verify list snapshot by limit param\n        for snap in xrange(2):\n            self.create_snapshot(self.volume_origin[\u0027id\u0027])\n\n        # list return limited elements\n        self._list_snapshots_by_param_limit(limit\u003d1)\n\n        # list return zero elements\n        self._list_snapshots_by_param_limit(limit\u003d0)\n\n        # list return all elements if requested limit exceed\n        snap_list \u003d self.snapshots_client.list_snapshots()[\u0027snapshots\u0027]\n        self._list_snapshots_by_param_limit(limit\u003d100000, expected_elements\u003dlen(snap_list))","commit_id":"f11d5991e899ca4d61d4d2780d92562caab04f91"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"feeb87e670acbade1fdc8d404d8039eef3987282","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        # Verify list snapshot by limit param"},{"line_number":193,"context_line":"        for snap in xrange(2):"},{"line_number":194,"context_line":"            self.create_snapshot(self.volume_origin[\u0027id\u0027])"},{"line_number":195,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":198,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a1b2928_9faf06e3","line":195,"range":{"start_line":195,"start_character":8,"end_line":195,"end_character":51},"in_reply_to":"7a1b2928_b1be87fd","updated":"2016-04-11 12:46:15.000000000","message":"Thanks for your comment, I\u0027ll add the two cases","commit_id":"f11d5991e899ca4d61d4d2780d92562caab04f91"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"ed220de3fbcc6b3671529e006198b29cbd949677","unresolved":false,"context_lines":[{"line_number":201,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":202,"context_line":"        # List returns zero elements"},{"line_number":203,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d0)"},{"line_number":204,"context_line":"        # List returns all elements when request limit exceeded"},{"line_number":205,"context_line":"        # snapshots number"},{"line_number":206,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d3, expected_elements\u003d2)"},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":209,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":13,"id":"5a18252c_c8bf9fa8","line":206,"range":{"start_line":204,"start_character":6,"end_line":206,"end_character":73},"updated":"2016-04-14 02:15:19.000000000","message":"This we need to make more realistic. expecting exactly 2 number of snapshot might fail in case if other tests also has created those.\n\nFor safer side please do like below-\n\n# list return all elements if requested limit exceed\n        snap_list \u003d self.snapshots_client.list_snapshots()[\u0027snapshots\u0027]\n        self._list_snapshots_by_param_limit(limit\u003d100000, expected_elements\u003dlen(snap_list))","commit_id":"d938fa155ae68f8e8617ff4bb8c395a74ac7ec06"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"aace7afa47fe843bc83bbef610695d9d0ae8cf4b","unresolved":false,"context_lines":[{"line_number":201,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":202,"context_line":"        # List returns zero elements"},{"line_number":203,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d0)"},{"line_number":204,"context_line":"        # List returns all elements when request limit exceeded"},{"line_number":205,"context_line":"        # snapshots number"},{"line_number":206,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d3, expected_elements\u003d2)"},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"    def cleanup_snapshot(self, snapshot):"},{"line_number":209,"context_line":"        # Delete the snapshot"}],"source_content_type":"text/x-python","patch_set":13,"id":"5a18252c_87e58ddd","line":206,"range":{"start_line":204,"start_character":6,"end_line":206,"end_character":73},"in_reply_to":"5a18252c_c8bf9fa8","updated":"2016-04-14 08:23:10.000000000","message":"Thanks for your commend, I understand, it\u0027ll be fixed","commit_id":"d938fa155ae68f8e8617ff4bb8c395a74ac7ec06"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"ed220de3fbcc6b3671529e006198b29cbd949677","unresolved":false,"context_lines":[{"line_number":215,"context_line":"class VolumesV1SnapshotTestJSON(VolumesV2SnapshotTestJSON):"},{"line_number":216,"context_line":"    _api_version \u003d 1"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"    @decorators.skip_because(bug\u003d\u00271540893\u0027)"},{"line_number":219,"context_line":"    @test.idempotent_id(\u002793f695d4-e069-4afa-9308-2fde882459b0\u0027)"},{"line_number":220,"context_line":"    def test_snapshot_list_param_limit(self):"},{"line_number":221,"context_line":"        super(VolumesV1SnapshotTestJSON, self).test_snapshot_list_param_limit()"}],"source_content_type":"text/x-python","patch_set":13,"id":"5a18252c_4838af3d","line":218,"range":{"start_line":218,"start_character":0,"end_line":218,"end_character":43},"updated":"2016-04-14 02:15:19.000000000","message":"Great. seems like you caught the v1 bug with those tests:)","commit_id":"d938fa155ae68f8e8617ff4bb8c395a74ac7ec06"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"aace7afa47fe843bc83bbef610695d9d0ae8cf4b","unresolved":false,"context_lines":[{"line_number":215,"context_line":"class VolumesV1SnapshotTestJSON(VolumesV2SnapshotTestJSON):"},{"line_number":216,"context_line":"    _api_version \u003d 1"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"    @decorators.skip_because(bug\u003d\u00271540893\u0027)"},{"line_number":219,"context_line":"    @test.idempotent_id(\u002793f695d4-e069-4afa-9308-2fde882459b0\u0027)"},{"line_number":220,"context_line":"    def test_snapshot_list_param_limit(self):"},{"line_number":221,"context_line":"        super(VolumesV1SnapshotTestJSON, self).test_snapshot_list_param_limit()"}],"source_content_type":"text/x-python","patch_set":13,"id":"5a18252c_a7da49e6","line":218,"range":{"start_line":218,"start_character":0,"end_line":218,"end_character":43},"in_reply_to":"5a18252c_4838af3d","updated":"2016-04-14 08:23:10.000000000","message":"How can I summit this patch if it fails on kilo version?\nIs there an option to skip on version?\nThanks for your help","commit_id":"d938fa155ae68f8e8617ff4bb8c395a74ac7ec06"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"4473c3dc7ce8a6c3b8456f314491749b431c2c18","unresolved":false,"context_lines":[{"line_number":16,"context_line":"from tempest import config"},{"line_number":17,"context_line":"from tempest import test"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"CONF \u003d config.CONF"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"1a122d0e_b823dfa6","line":19,"updated":"2016-05-03 08:56:08.000000000","message":"Please, next time, do not add empty line like this.","commit_id":"f8f91911331c8a4b244c5d0e1668311f3641f682"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"4473c3dc7ce8a6c3b8456f314491749b431c2c18","unresolved":false,"context_lines":[{"line_number":59,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":60,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    def _list_snapshots_by_param_limit(self, limit, expected_elements\u003dNone):"},{"line_number":63,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("}],"source_content_type":"text/x-python","patch_set":16,"id":"1a122d0e_b85abf28","line":62,"updated":"2016-05-03 08:56:08.000000000","message":"Why do you introduce a new method, since it\u0027s going to be called only once ?","commit_id":"f8f91911331c8a4b244c5d0e1668311f3641f682"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"ecaeb5a95b6cf9a703ae018fd3af251ca7c11a52","unresolved":false,"context_lines":[{"line_number":59,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":60,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    def _list_snapshots_by_param_limit(self, limit, expected_elements\u003dNone):"},{"line_number":63,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":64,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":65,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("}],"source_content_type":"text/x-python","patch_set":16,"id":"1a122d0e_1900f465","line":62,"in_reply_to":"1a122d0e_b85abf28","updated":"2016-05-04 08:47:32.000000000","message":"Hi Jordan,\nThe method is called twice: lines 200 \u0026 204\nThere was another call when limit\u003d0, but I removed this case, since it caught 2 cinder bugs. One in api V1 and second on kilo V2. Skip on api V1 is not an issue but I don\u0027t know if there is an option to skip on kilo version. So until it\u0027ll be fixed on kilo I removed the limit\u003d0 filter case.","commit_id":"f8f91911331c8a4b244c5d0e1668311f3641f682"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"4473c3dc7ce8a6c3b8456f314491749b431c2c18","unresolved":false,"context_lines":[{"line_number":66,"context_line":"            limit\u003dlimit)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        # Only if expected_elements is None, set it to limit argument"},{"line_number":68,"context_line":"        # otherwise, use expected_elements argument"},{"line_number":69,"context_line":"        expected_elements \u003d expected_elements or limit"},{"line_number":70,"context_line":"        self.assertEqual(expected_elements, len(fetched_snap_list))"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a122d0e_38686f87","line":69,"updated":"2016-05-03 08:56:08.000000000","message":"Why is this needed ? It seems expected_elements can\"t ever be None since it is called line 204 with len(snap_list) and you created 2 snapshots just before.","commit_id":"f8f91911331c8a4b244c5d0e1668311f3641f682"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"ecaeb5a95b6cf9a703ae018fd3af251ca7c11a52","unresolved":false,"context_lines":[{"line_number":66,"context_line":"            limit\u003dlimit)[\u0027snapshots\u0027]"},{"line_number":67,"context_line":"        # Only if expected_elements is None, set it to limit argument"},{"line_number":68,"context_line":"        # otherwise, use expected_elements argument"},{"line_number":69,"context_line":"        expected_elements \u003d expected_elements or limit"},{"line_number":70,"context_line":"        self.assertEqual(expected_elements, len(fetched_snap_list))"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    @test.idempotent_id(\u0027b467b54c-07a4-446d-a1cf-651dedcc3ff1\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a122d0e_79f7688c","line":69,"in_reply_to":"1a122d0e_38686f87","updated":"2016-05-04 08:47:32.000000000","message":"As you can see in line 200, _list_snapshots_by_param_limit method is called limit\u003d1 and expected_elements\u003dNone","commit_id":"f8f91911331c8a4b244c5d0e1668311f3641f682"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ddc0cb625f6aba3ef0019992d6d47263dbb35572","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, limit, expected_elements\u003dNone):"},{"line_number":62,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":63,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":64,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_73ca4a51","line":61,"updated":"2016-05-06 09:25:09.000000000","message":"Please make this expected_elements a mandatory argument (no default value).\n\nThe caller of this method should know how many elements to expect and should not let this method picks the correct value. Let\u0027s be explicit.","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"2234a2c2a338b4c2f00b8497b3f4430f25388c60","unresolved":false,"context_lines":[{"line_number":58,"context_line":"                      (\u0027details\u0027 if with_detail else \u0027\u0027, key)"},{"line_number":59,"context_line":"                self.assertEqual(params[key], snap[key], msg)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def _list_snapshots_by_param_limit(self, limit, expected_elements\u003dNone):"},{"line_number":62,"context_line":"        \"\"\"list snapshots by limit param\"\"\""},{"line_number":63,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":64,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_2ccf7d80","line":61,"in_reply_to":"dab17558_73ca4a51","updated":"2016-05-09 09:40:13.000000000","message":"Thanks for your comments, I appreciate your time and effort","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ddc0cb625f6aba3ef0019992d6d47263dbb35572","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":64,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":65,"context_line":"            limit\u003dlimit)[\u0027snapshots\u0027]"},{"line_number":66,"context_line":"        # Only if expected_elements is None, set it to limit argument"},{"line_number":67,"context_line":"        # otherwise, use expected_elements argument"},{"line_number":68,"context_line":"        expected_elements \u003d expected_elements or limit"},{"line_number":69,"context_line":"        self.assertEqual(expected_elements, len(fetched_snap_list))"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_f303da74","line":66,"updated":"2016-05-06 09:25:09.000000000","message":"This \"optimization\" (did you do that to save the caller from providing a explicit value to expected_elements ?), hurts readability.","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"2234a2c2a338b4c2f00b8497b3f4430f25388c60","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        # Get snapshots list using limit parameter"},{"line_number":64,"context_line":"        fetched_snap_list \u003d self.snapshots_client.list_snapshots("},{"line_number":65,"context_line":"            limit\u003dlimit)[\u0027snapshots\u0027]"},{"line_number":66,"context_line":"        # Only if expected_elements is None, set it to limit argument"},{"line_number":67,"context_line":"        # otherwise, use expected_elements argument"},{"line_number":68,"context_line":"        expected_elements \u003d expected_elements or limit"},{"line_number":69,"context_line":"        self.assertEqual(expected_elements, len(fetched_snap_list))"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_6cc98578","line":66,"in_reply_to":"dab17558_f303da74","updated":"2016-05-09 09:40:13.000000000","message":"Done","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ddc0cb625f6aba3ef0019992d6d47263dbb35572","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":193,"context_line":"    def test_snapshot_list_param_limit(self):"},{"line_number":194,"context_line":"        # Verify list snapshot by limit param"},{"line_number":195,"context_line":"        # Create 2 snapshots"},{"line_number":196,"context_line":"        for snap in xrange(2):"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_b39f925e","line":193,"updated":"2016-05-06 09:25:09.000000000","message":"I know what bugs me from the beginning now. You have 2 test cases here. One is with limit \u003d 1, the other is with limit equals infinite. It really should be 2 different test cases (please do the snapshot creation in the resource_setup method)","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"2234a2c2a338b4c2f00b8497b3f4430f25388c60","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        self.cleanup_snapshot(snapshot)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"    @test.idempotent_id(\u0027db4d8e0a-7a2e-41cc-a712-961f6844e896\u0027)"},{"line_number":193,"context_line":"    def test_snapshot_list_param_limit(self):"},{"line_number":194,"context_line":"        # Verify list snapshot by limit param"},{"line_number":195,"context_line":"        # Create 2 snapshots"},{"line_number":196,"context_line":"        for snap in xrange(2):"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_4cc4815e","line":193,"in_reply_to":"dab17558_b39f925e","updated":"2016-05-09 09:40:13.000000000","message":"Done","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ddc0cb625f6aba3ef0019992d6d47263dbb35572","unresolved":false,"context_lines":[{"line_number":198,"context_line":"        # TODO(lkuchlan): Once Bug#1540893 will be fixed add a filter"},{"line_number":199,"context_line":"        # case - limit\u003d0."},{"line_number":200,"context_line":"        # List returns limited elements"},{"line_number":201,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":202,"context_line":"        # List returns all elements when request limit exceeded"},{"line_number":203,"context_line":"        # snapshots number"},{"line_number":204,"context_line":"        snap_list \u003d self.snapshots_client.list_snapshots()[\u0027snapshots\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_b376723c","line":201,"updated":"2016-05-06 09:25:09.000000000","message":"Please call the method with an explicit expected_elements\u003d1 kwarg. This way it is very clear what you expect.","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"2234a2c2a338b4c2f00b8497b3f4430f25388c60","unresolved":false,"context_lines":[{"line_number":198,"context_line":"        # TODO(lkuchlan): Once Bug#1540893 will be fixed add a filter"},{"line_number":199,"context_line":"        # case - limit\u003d0."},{"line_number":200,"context_line":"        # List returns limited elements"},{"line_number":201,"context_line":"        self._list_snapshots_by_param_limit(limit\u003d1)"},{"line_number":202,"context_line":"        # List returns all elements when request limit exceeded"},{"line_number":203,"context_line":"        # snapshots number"},{"line_number":204,"context_line":"        snap_list \u003d self.snapshots_client.list_snapshots()[\u0027snapshots\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"dab17558_aca1adab","line":201,"in_reply_to":"dab17558_b376723c","updated":"2016-05-09 09:40:13.000000000","message":"Done","commit_id":"70e14dc76dc2b7b2729c84c3902ed9718a5bc3e5"},{"author":{"_account_id":5803,"name":"Attila Fazekas","email":"afazekas@redhat.com","username":"afazekas"},"change_message_id":"78d3010e426a3c2969f2c7fce7a9e852f6b3c138","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        cls.name_field \u003d cls.special_fields[\u0027name_field\u0027]"},{"line_number":37,"context_line":"        cls.descrip_field \u003d cls.special_fields[\u0027descrip_field\u0027]"},{"line_number":38,"context_line":"        # Create 2 snapshots"},{"line_number":39,"context_line":"        for snap in xrange(2):"},{"line_number":40,"context_line":"            cls.create_snapshot(cls.volume_origin[\u0027id\u0027])"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def _detach(self, volume_id):"}],"source_content_type":"text/x-python","patch_set":18,"id":"dab17558_d9b5c7ee","line":39,"updated":"2016-05-09 15:00:37.000000000","message":"loop for 2 iteration, with blank variable.\n\n- you do not need loop here\n- If you are not using a loop variable it should be named as   \u0027_\u0027 .","commit_id":"4c151ac1a4be07f3c8fad29effe00b74e53ad7df"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"c802a08aa45730bf555088105f4a4833ee9ce535","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        cls.name_field \u003d cls.special_fields[\u0027name_field\u0027]"},{"line_number":37,"context_line":"        cls.descrip_field \u003d cls.special_fields[\u0027descrip_field\u0027]"},{"line_number":38,"context_line":"        # Create 2 snapshots"},{"line_number":39,"context_line":"        for snap in xrange(2):"},{"line_number":40,"context_line":"            cls.create_snapshot(cls.volume_origin[\u0027id\u0027])"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def _detach(self, volume_id):"}],"source_content_type":"text/x-python","patch_set":18,"id":"dab17558_ba0d5139","line":39,"in_reply_to":"dab17558_d9b5c7ee","updated":"2016-05-09 15:41:49.000000000","message":"Thanks for your comment, it\u0027ll be fixed","commit_id":"4c151ac1a4be07f3c8fad29effe00b74e53ad7df"}]}