)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b56c095c94568d174b1cd82369b54c30606c2e27","unresolved":false,"context_lines":[{"line_number":17,"context_line":"file not being included in a new hashes.pkl when new suffix hashes"},{"line_number":18,"context_line":"are calculated, which may be considered a bug to be fixed in a later"},{"line_number":19,"context_line":"patch, but this patch is simply increasing test coverage of the"},{"line_number":20,"context_line":"existing code."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Change-Id: I2d09253b824f011de789aa8f0fe02ede8aeeb937"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"5a3905b3_a551e9da","line":20,"updated":"2017-01-24 22:00:07.000000000","message":"I think committing a test to enshrine a unquestionably bad behavior is a super fine line - I think some of these tests would look better on the change that includes the fix","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"4264e6e5dd246a1c862bf46981a92abdf27e30ff","unresolved":false,"context_lines":[{"line_number":6054,"context_line":"            suffix_dir \u003d os.path.dirname(df._datadir)"},{"line_number":6055,"context_line":"            suffix \u003d os.path.basename(suffix_dir)"},{"line_number":6056,"context_line":"            hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":6057,"context_line":"            self.assertIn(suffix, hashes)  # sanity"},{"line_number":6058,"context_line":""},{"line_number":6059,"context_line":"    def test_invalidate_hash_consolidation(self):"},{"line_number":6060,"context_line":"        def assert_consolidation(suffixes):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a3905b3_a0c1d730","line":6057,"updated":"2017-01-19 09:10:17.000000000","message":"Not related to your changes - guess we forgot to add some checks here? Maybe these could be added - https://gist.github.com/MahatiC/0b80574a519294693679596884a843a5","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"73ede3866fd36bdfed71bfff2a425f27d2f2244b","unresolved":false,"context_lines":[{"line_number":6137,"context_line":"            # consolidate hashes"},{"line_number":6138,"context_line":"            assert_consolidation([suffix, suffix2])"},{"line_number":6139,"context_line":""},{"line_number":6140,"context_line":"    def test_consolidate_hashes_raises_exception(self):"},{"line_number":6141,"context_line":"        # verify that if consolidate_hashes raises an exception then suffixes"},{"line_number":6142,"context_line":"        # are rehashed and a hashes.pkl is written"},{"line_number":6143,"context_line":"        for policy in self.iter_policies():"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a3c09a3_e4f07769","line":6140,"updated":"2017-01-18 13:15:54.000000000","message":"ideally this test and the below two consolidate_hashes tests could be combined, after separating the common bits. But it\u0027d still be fine by me if we just keep out the common part into a reusable function (refer comment on line#6202 below)","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"4264e6e5dd246a1c862bf46981a92abdf27e30ff","unresolved":false,"context_lines":[{"line_number":6137,"context_line":"            # consolidate hashes"},{"line_number":6138,"context_line":"            assert_consolidation([suffix, suffix2])"},{"line_number":6139,"context_line":""},{"line_number":6140,"context_line":"    def test_consolidate_hashes_raises_exception(self):"},{"line_number":6141,"context_line":"        # verify that if consolidate_hashes raises an exception then suffixes"},{"line_number":6142,"context_line":"        # are rehashed and a hashes.pkl is written"},{"line_number":6143,"context_line":"        for policy in self.iter_policies():"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a3905b3_e58cd137","line":6140,"in_reply_to":"7a3c09a3_a8a71da5","updated":"2017-01-19 09:10:17.000000000","message":"Sure, they\u0027re definitely more readable! I wasn\u0027t suggesting to merge them in the current state, was hoping to restructure tests - more on that in my below comment.","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cc8aae309b50d8513a4bf398dd247f6ddb83564","unresolved":false,"context_lines":[{"line_number":6137,"context_line":"            # consolidate hashes"},{"line_number":6138,"context_line":"            assert_consolidation([suffix, suffix2])"},{"line_number":6139,"context_line":""},{"line_number":6140,"context_line":"    def test_consolidate_hashes_raises_exception(self):"},{"line_number":6141,"context_line":"        # verify that if consolidate_hashes raises an exception then suffixes"},{"line_number":6142,"context_line":"        # are rehashed and a hashes.pkl is written"},{"line_number":6143,"context_line":"        for policy in self.iter_policies():"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a3c09a3_a8a71da5","line":6140,"in_reply_to":"7a3c09a3_e4f07769","updated":"2017-01-18 14:51:39.000000000","message":"I created two separate new tests in the hope that they would be more readable. I sometimes find it hard to grok some existing very long tests that flow through lots of states. That said, what I propose below are already quite long!\n\nAs I comment below, if you can see a nice way to share some common setup code then please suggest a diff.","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"73ede3866fd36bdfed71bfff2a425f27d2f2244b","unresolved":false,"context_lines":[{"line_number":6188,"context_line":"            with open(path, mode) as fd:"},{"line_number":6189,"context_line":"                yield fd"},{"line_number":6190,"context_line":""},{"line_number":6191,"context_line":"        for policy in self.iter_policies():"},{"line_number":6192,"context_line":"            df_mgr \u003d self.df_router[policy]"},{"line_number":6193,"context_line":"            # create something to hash"},{"line_number":6194,"context_line":"            df \u003d df_mgr.get_diskfile(\u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027, \u0027o\u0027,"},{"line_number":6195,"context_line":"                                     policy\u003dpolicy)"},{"line_number":6196,"context_line":"            df.delete(self.ts())"},{"line_number":6197,"context_line":"            suffix_dir \u003d os.path.dirname(df._datadir)"},{"line_number":6198,"context_line":"            suffix \u003d os.path.basename(suffix_dir)"},{"line_number":6199,"context_line":"            part_dir \u003d os.path.dirname(suffix_dir)"},{"line_number":6200,"context_line":"            hashes_file \u003d os.path.join(part_dir, diskfile.HASH_FILE)"},{"line_number":6201,"context_line":"            invalidations_file \u003d os.path.join("},{"line_number":6202,"context_line":"                part_dir, diskfile.HASH_INVALIDATIONS_FILE)"},{"line_number":6203,"context_line":"            with mock.patch.object(df_mgr, \u0027_hash_suffix\u0027,"},{"line_number":6204,"context_line":"                                   return_value\u003d\u0027fake hash\u0027):"},{"line_number":6205,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a3c09a3_a4c3cf36","line":6202,"range":{"start_line":6191,"start_character":8,"end_line":6202,"end_character":59},"updated":"2017-01-18 13:15:54.000000000","message":"We\u0027re reusing this bit at least in 4 tests here, worth moving it to a function? If you would like me to, I could push up a patch set over here.","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"4264e6e5dd246a1c862bf46981a92abdf27e30ff","unresolved":false,"context_lines":[{"line_number":6188,"context_line":"            with open(path, mode) as fd:"},{"line_number":6189,"context_line":"                yield fd"},{"line_number":6190,"context_line":""},{"line_number":6191,"context_line":"        for policy in self.iter_policies():"},{"line_number":6192,"context_line":"            df_mgr \u003d self.df_router[policy]"},{"line_number":6193,"context_line":"            # create something to hash"},{"line_number":6194,"context_line":"            df \u003d df_mgr.get_diskfile(\u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027, \u0027o\u0027,"},{"line_number":6195,"context_line":"                                     policy\u003dpolicy)"},{"line_number":6196,"context_line":"            df.delete(self.ts())"},{"line_number":6197,"context_line":"            suffix_dir \u003d os.path.dirname(df._datadir)"},{"line_number":6198,"context_line":"            suffix \u003d os.path.basename(suffix_dir)"},{"line_number":6199,"context_line":"            part_dir \u003d os.path.dirname(suffix_dir)"},{"line_number":6200,"context_line":"            hashes_file \u003d os.path.join(part_dir, diskfile.HASH_FILE)"},{"line_number":6201,"context_line":"            invalidations_file \u003d os.path.join("},{"line_number":6202,"context_line":"                part_dir, diskfile.HASH_INVALIDATIONS_FILE)"},{"line_number":6203,"context_line":"            with mock.patch.object(df_mgr, \u0027_hash_suffix\u0027,"},{"line_number":6204,"context_line":"                                   return_value\u003d\u0027fake hash\u0027):"},{"line_number":6205,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a3905b3_85e78d3f","line":6202,"range":{"start_line":6191,"start_character":8,"end_line":6202,"end_character":59},"in_reply_to":"7a3c09a3_68e53507","updated":"2017-01-19 09:10:17.000000000","message":"Good points! I could only come up with the latter case you suggested here (have a separate class) OR could create a global function which will also require doing things we\u0027re doing in setUp - but it could be considered in future when we have more tests based on the same setup maybe. Because right now, moving it would ultimately make little difference for the repetition case that I\u0027d like to see addressed! Guess I spoke too soon ;)\n\nThanks for pointing out in detail.","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8cc8aae309b50d8513a4bf398dd247f6ddb83564","unresolved":false,"context_lines":[{"line_number":6188,"context_line":"            with open(path, mode) as fd:"},{"line_number":6189,"context_line":"                yield fd"},{"line_number":6190,"context_line":""},{"line_number":6191,"context_line":"        for policy in self.iter_policies():"},{"line_number":6192,"context_line":"            df_mgr \u003d self.df_router[policy]"},{"line_number":6193,"context_line":"            # create something to hash"},{"line_number":6194,"context_line":"            df \u003d df_mgr.get_diskfile(\u0027sda1\u0027, \u00270\u0027, \u0027a\u0027, \u0027c\u0027, \u0027o\u0027,"},{"line_number":6195,"context_line":"                                     policy\u003dpolicy)"},{"line_number":6196,"context_line":"            df.delete(self.ts())"},{"line_number":6197,"context_line":"            suffix_dir \u003d os.path.dirname(df._datadir)"},{"line_number":6198,"context_line":"            suffix \u003d os.path.basename(suffix_dir)"},{"line_number":6199,"context_line":"            part_dir \u003d os.path.dirname(suffix_dir)"},{"line_number":6200,"context_line":"            hashes_file \u003d os.path.join(part_dir, diskfile.HASH_FILE)"},{"line_number":6201,"context_line":"            invalidations_file \u003d os.path.join("},{"line_number":6202,"context_line":"                part_dir, diskfile.HASH_INVALIDATIONS_FILE)"},{"line_number":6203,"context_line":"            with mock.patch.object(df_mgr, \u0027_hash_suffix\u0027,"},{"line_number":6204,"context_line":"                                   return_value\u003d\u0027fake hash\u0027):"},{"line_number":6205,"context_line":"                df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a3c09a3_68e53507","line":6202,"range":{"start_line":6191,"start_character":8,"end_line":6202,"end_character":59},"in_reply_to":"7a3c09a3_a4c3cf36","updated":"2017-01-18 14:51:39.000000000","message":"I considered that but given that the tests refer to df_mgr, suffix, suffix_dir, part_dir, hashes_file, invalidations_file, then any helper method such as:\n\n  _create_tombstone(policy)\n\nwould have to return a large tuple, which isn\u0027t great, or a dict with those values, which I suspect would make the tests less readable.\n\nOR, I could break these tests into a separate class and have a dedicated setUp method to create instance vars, but here the repeated setup seems small compared to the length of the tests.\n\nUsually I like to re-use setup code when possible, but here I\u0027m not sure. Please leave a diff if you can see a nice way.\n\nThanks","commit_id":"59898b31d4d90d3c1a009ee25208b25c7f02440b"},{"author":{"_account_id":12279,"name":"Janie Richling","email":"jrichli@us.ibm.com","username":"jrichli"},"change_message_id":"435a6cedd0ec6d4546a3ab8db7f21ef4e3f6a0cb","unresolved":false,"context_lines":[{"line_number":6212,"context_line":"            # OSError opening hashes.pkl"},{"line_number":6213,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027) \\"},{"line_number":6214,"context_line":"                    as fake_write_pickle:"},{"line_number":6215,"context_line":"                with mock.patch(\u0027swift.obj.diskfile.open\u0027, fake_open):"},{"line_number":6216,"context_line":"                    hashes \u003d df_mgr.consolidate_hashes(part_dir)"},{"line_number":6217,"context_line":"            self.assertEqual({}, hashes)"},{"line_number":6218,"context_line":"            # verify there is still the same hashes.pkl (BUT return value of {}"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_314a1856","line":6215,"updated":"2017-01-29 04:58:52.000000000","message":"I think fake_open is meant to mock os.open(path,mode) instead of diskfile.open().  Is that correct?  As the mock stands, tests succeed when I run tox -e py27.  But if I run using ./.unittests (which uses nosetests) then I get an error: AttributeError: \u003cmodule \u0027swift.obj.diskfile\u0027 from \u0027/home/swift/projects/master/swift/swift/obj/diskfile.py\u0027\u003e does not have the attribute \u0027open\u0027","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b56c095c94568d174b1cd82369b54c30606c2e27","unresolved":false,"context_lines":[{"line_number":6216,"context_line":"                    hashes \u003d df_mgr.consolidate_hashes(part_dir)"},{"line_number":6217,"context_line":"            self.assertEqual({}, hashes)"},{"line_number":6218,"context_line":"            # verify there is still the same hashes.pkl (BUT return value of {}"},{"line_number":6219,"context_line":"            # can result in no rewrite of hashes.pkl in _get_hashes ? )"},{"line_number":6220,"context_line":"            self.assertTrue(os.path.exists(hashes_file))"},{"line_number":6221,"context_line":"            self.assertEqual(0, fake_write_pickle.call_count)"},{"line_number":6222,"context_line":"            with open(hashes_file, \u0027rb\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a3905b3_8ab87a2c","line":6219,"updated":"2017-01-24 22:00:07.000000000","message":"this commentary is unrelated to this tests assertions - the bad behavior is covered in a different test.","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f26544c35c640fb5654dc44a68c5a06845597919","unresolved":false,"context_lines":[{"line_number":7392,"context_line":"            hashes_file \u003d os.path.join(part_dir, diskfile.HASH_FILE)"},{"line_number":7393,"context_line":"            hashes \u003d df_mgr.get_hashes(self.existing_device, \u00270\u0027, [], policy)"},{"line_number":7394,"context_line":"            self.assertIn(suffix, hashes)  # sanity"},{"line_number":7395,"context_line":"            # now make hashes file unreadable"},{"line_number":7396,"context_line":"            os.chmod(hashes_file, 0)"},{"line_number":7397,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.write_pickle\u0027)\\"},{"line_number":7398,"context_line":"                    as fake_write_pickle:"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a3c09a3_bd9759ea","line":7395,"updated":"2017-01-18 16:18:47.000000000","message":"I modified this test to use an unreadbale file because it turns out that even with the fix [1] a dir will cause the hashing to fail - renamer cannot rename a dir.\n\n[1] https://review.openstack.org/422076","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b56c095c94568d174b1cd82369b54c30606c2e27","unresolved":false,"context_lines":[{"line_number":7401,"context_line":"            self.assertEqual(hashes, {})"},{"line_number":7402,"context_line":"            # but no attempt to write a pickle file"},{"line_number":7403,"context_line":"            self.assertEqual(0, fake_write_pickle.call_count)"},{"line_number":7404,"context_line":"            self.assertTrue(os.path.exists(hashes_file))"},{"line_number":7405,"context_line":""},{"line_number":7406,"context_line":"            # now add another suffix dir and rehash"},{"line_number":7407,"context_line":"            df2 \u003d self.get_different_suffix_df("}],"source_content_type":"text/x-python","patch_set":2,"id":"5a3905b3_c5cef5b6","line":7404,"updated":"2017-01-24 22:00:07.000000000","message":"this is not going to end well :\\","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b56c095c94568d174b1cd82369b54c30606c2e27","unresolved":false,"context_lines":[{"line_number":7418,"context_line":"            with open(hashes_file) as f:"},{"line_number":7419,"context_line":"                self.assertEqual(hashes, pickle.load(f))"},{"line_number":7420,"context_line":"            # and first suffix did not get included in new hashes.pkl !!"},{"line_number":7421,"context_line":"            self.assertNotIn(suffix, hashes)"},{"line_number":7422,"context_line":""},{"line_number":7423,"context_line":"    def test_get_hashes_zero_bytes_pickle(self):"},{"line_number":7424,"context_line":"        for policy in self.iter_policies():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a3905b3_2a5ce6ec","line":7421,"updated":"2017-01-24 22:00:07.000000000","message":"I really don\u0027t like having a test for this behavior - this isn\u0027t \"questionable\" - this is a great test to demonstrate the current behavior is WRONG\n\nIf we want to merge this test onto master w/o the follow up fix it should make the *correct* assertion - and be decorated with some sort of expected failure - and probably reference an open bug report.\n\nI would suggest this test just belongs in the follow up fix and having it here doesn\u0027t make the diff for the follow up fix more obvious.","commit_id":"32383103d1079c8f4d2269c9cca2ef651e37a0c1"}]}