)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c371a16c_0a954493","updated":"2026-05-15 17:35:00.000000000","message":"thank you for working on this; I think it\u0027s really unfortunate that we don\u0027t really have ANY tests for dispersion_report - it\u0027s going to make it significantly harder to maintain and improve.  As such I think it\u0027s a kind of \"bug\" that we *completely break* `object_dispersion_report` w/o any unittests failing.\n\n\u003e Fixing bugs is HUGELY valuable - the only thing which has a higher cost than the value of fixing a bug - is adding a new bug - if it’s broken and this change makes it fixed (without breaking anything else) you have a winner!\n\nhttps://docs.openstack.org/swift/latest/contributor/review_guidelines.html#scoring\n\nIn order to say if this should merge as-is I have to ask what\u0027s being fixed and if anything is worse; subjectively (I could be wrong!) I think this is fixing:\n\n - you can\u0027t easily break the two helper methods in dispersion_report, missing_string and get_error_log w/o unittests failing \n \n^ that\u0027s not nothing!  KUDOS!\n\nWhat I think is made worse:\n\n - there\u0027s a lot of \"lines\" in `test_dispersion_report` that may give you a false sense of security\n \nconsider this:\n \n```\nvagrant@saio:~$ swift-dispersion-report \nTraceback (most recent call last):\n  File \"/usr/local/bin/swift-dispersion-report\", line 6, in \u003cmodule\u003e\n    sys.exit(main())\n  File \"/vagrant/swift/swift/cli/dispersion_report.py\", line 357, in main\n    output \u003d generate_report(conf, options.policy_name)\n  File \"/vagrant/swift/swift/cli/dispersion_report.py\", line 364, in generate_report\n    asdf\nNameError: name \u0027asdf\u0027 is not defined\nvagrant@saio:~$ pytest swift/test/unit/cli/test_dispersion_report.py \n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d test session starts \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\nplatform linux -- Python 3.10.12, pytest-9.0.2, pluggy-1.6.0 -- /usr/bin/python3\ncachedir: .pytest_cache\nrootdir: /home/vagrant/swift\nconfigfile: tox.ini\nplugins: cov-7.1.0\ncollected 13 items                                                                                                                                                                                                                                     \n\nswift/test/unit/cli/test_dispersion_report.py::TestGetErrorLog::test_error_log_404_not_logged_when_debug_off PASSED                                                                                                                              [  7%]\nswift/test/unit/cli/test_dispersion_report.py::TestGetErrorLog::test_error_log_non_http_exception PASSED                                                                                                                                         [ 15%]\nswift/test/unit/cli/test_dispersion_report.py::TestGetErrorLog::test_error_log_returns_callable PASSED                                                                                                                                           [ 23%]\nswift/test/unit/cli/test_dispersion_report.py::TestGetErrorLog::test_error_log_unmounted_device PASSED                                                                                                                                           [ 30%]\nswift/test/unit/cli/test_dispersion_report.py::TestMissingString::test_all_copies_missing PASSED                                                                                                                                                 [ 38%]\nswift/test/unit/cli/test_dispersion_report.py::TestMissingString::test_multiple_partitions_multiple_copies_missing PASSED                                                                                                                        [ 46%]\nswift/test/unit/cli/test_dispersion_report.py::TestMissingString::test_multiple_partitions_single_copy_missing PASSED                                                                                                                            [ 53%]\nswift/test/unit/cli/test_dispersion_report.py::TestMissingString::test_one_copy_remaining PASSED                                                                                                                                                 [ 61%]\nswift/test/unit/cli/test_dispersion_report.py::TestMissingString::test_single_partition_single_copy_missing PASSED                                                                                                                               [ 69%]\nswift/test/unit/cli/test_dispersion_report.py::TestGenerateReport::test_generate_report_is_callable PASSED                                                                                                                                       [ 76%]\nswift/test/unit/cli/test_dispersion_report.py::TestContainerDispersionReport::test_container_report_is_callable PASSED                                                                                                                           [ 84%]\nswift/test/unit/cli/test_dispersion_report.py::TestObjectDispersionReport::test_object_report_is_callable PASSED                                                                                                                                 [ 92%]\nswift/test/unit/cli/test_dispersion_report.py::TestMainFunction::test_main_is_callable PASSED                                                                                                                                                    [100%]\n\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d 13 passed, 1 warning in 0.26s \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\n```\n\nthat\u0027s a LOT of passing tests with very official sounding names to support the idea that `swift-dispersion-report` is not broken; when in-fact it could not be further from the truth.  Compared to master:\n\n```\nvagrant@saio:~$ swift-dispersion-report \nTraceback (most recent call last):\n  File \"/usr/local/bin/swift-dispersion-report\", line 6, in \u003cmodule\u003e\n    sys.exit(main())\n  File \"/vagrant/swift/swift/cli/dispersion_report.py\", line 356, in main\n    output \u003d generate_report(conf, options.policy_name)\n  File \"/vagrant/swift/swift/cli/dispersion_report.py\", line 363, in generate_report\n    asdf\nNameError: name \u0027asdf\u0027 is not defined\nvagrant@saio:~$ pytest swift/test/unit/cli/test_dispersion_report.py \n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d test session starts \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\nplatform linux -- Python 3.10.12, pytest-9.0.2, pluggy-1.6.0 -- /usr/bin/python3\ncachedir: .pytest_cache\nrootdir: /home/vagrant/swift\nconfigfile: tox.ini\nplugins: cov-7.1.0\ncollected 1 item                                                                                                                                                                                                                                       \n\nswift/test/unit/cli/test_dispersion_report.py::TestDispersionReport::test_placeholder PASSED                                                                                                                                                     [100%]\n\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d 1 passed, 1 warning in 0.34s \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\nvagrant@saio:~$ \n```\n\n`test_placeholer` is NOT going to give any reasonable maintainer ANY confidence that the unittests are doing ANYTHING to assert the correctness of the `dispersion_report` module; and rather HIGHLIGHT that they need to go do some additional verification.\n\nI think that difference in behavior between master and this change is a *kind of* \"worse\" (subjectively!  I could be wrong!)\n\nSo I\u0027d like to ask if you\u0027d be willing to *actually* test swift-dispersion-report - something that mocks out direct_client/SimpleClient and actually *runs* the code being tested so that stuff like `NameError`s in this module would trigger test failures - that would be a HUGE FIX IMHO.","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"}],"test/unit/cli/test_dispersion_report.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":16,"context_line":"from swift.cli import dispersion_report"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"class TestGetErrorLog(unittest.TestCase):"},{"line_number":20,"context_line":"    \"\"\"Tests for the get_error_log function\"\"\""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"    def setUp(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"b2ab50f1_156a97ee","line":19,"updated":"2026-05-15 17:35:00.000000000","message":"ok, I think there is one TestCase for each top-level function:\n\n```\n(vagrant-swift-all-in-one) cgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ ag \"^def \" swift/cli/dispersion_report.py \n42:def get_error_log(prefix):\n69:def container_dispersion_report(coropool, connpool, account, container_ring,\n166:def object_dispersion_report(coropool, connpool, account, object_ring,\n281:def missing_string(partition_count, missing_copies, copy_count):\n306:def main():\n362:def generate_report(conf, policy_name\u003dNone):\n```\n\n```\n(vagrant-swift-all-in-one) cgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ ag \"^class \" test/unit/cli/test_dispersion_report.py \n19:class TestGetErrorLog(unittest.TestCase):\n80:class TestMissingString(unittest.TestCase):\n116:class TestGenerateReport(unittest.TestCase):\n132:class TestContainerDispersionReport(unittest.TestCase):\n153:class TestObjectDispersionReport(unittest.TestCase):\n173:class TestMainFunction(unittest.TestCase):\n```","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":16,"context_line":"from swift.cli import dispersion_report"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"class TestGetErrorLog(unittest.TestCase):"},{"line_number":20,"context_line":"    \"\"\"Tests for the get_error_log function\"\"\""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"    def setUp(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"529b4c53_464de095","line":19,"in_reply_to":"b2ab50f1_156a97ee","updated":"2026-05-27 15:01:02.000000000","message":"Done","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":22,"context_line":"    def setUp(self):"},{"line_number":23,"context_line":"        \"\"\"Reset global state before each test\"\"\""},{"line_number":24,"context_line":"        dispersion_report.unmounted \u003d []"},{"line_number":25,"context_line":"        dispersion_report.notfound \u003d []"},{"line_number":26,"context_line":"        dispersion_report.debug \u003d False"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def tearDown(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"773d186a_85711ab3","line":25,"updated":"2026-05-15 17:35:00.000000000","message":"these module level globals are seemingly used to memoize the first report of an eror on a device; what\u0027s weird is that `get_error_log` is already a closure - so I don\u0027t really see why they would need to be globals.","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":22,"context_line":"    def setUp(self):"},{"line_number":23,"context_line":"        \"\"\"Reset global state before each test\"\"\""},{"line_number":24,"context_line":"        dispersion_report.unmounted \u003d []"},{"line_number":25,"context_line":"        dispersion_report.notfound \u003d []"},{"line_number":26,"context_line":"        dispersion_report.debug \u003d False"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def tearDown(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5bb9e712_5fbf5f33","line":25,"in_reply_to":"773d186a_85711ab3","updated":"2026-05-27 15:01:02.000000000","message":"Done","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":47,"context_line":"        exc.http_port \u003d 6000"},{"line_number":48,"context_line":"        exc.http_device \u003d \u0027sda\u0027"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"        # Just verify it doesn\u0027t raise an exception"},{"line_number":51,"context_line":"        error_log(exc)"},{"line_number":52,"context_line":"        # Check that the device was added to unmounted list"},{"line_number":53,"context_line":"        self.assertIn(\u0027192.168.1.1:6000/sda\u0027, dispersion_report.unmounted)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3d0fcaf1_b9e9a9ff","line":50,"updated":"2026-05-15 17:35:00.000000000","message":"the \"behavior\" of the `error_log` IMHO is quite clear and we could test it by capturing stderr:\n\n```\ndiff --git a/test/unit/cli/test_dispersion_report.py b/test/unit/cli/test_dispersion_report.py\nindex a9f436ede3..f4d9a7cbd8 100644\n--- a/test/unit/cli/test_dispersion_report.py\n+++ b/test/unit/cli/test_dispersion_report.py\n@@ -10,8 +10,9 @@\n # License for the specific language governing permissions and limitations\n # under the License.\n \n+from io import StringIO\n import unittest\n-from unittest.mock import Mock\n+from unittest import mock\n \n from swift.cli import dispersion_report\n \n@@ -36,28 +37,35 @@ class TestGetErrorLog(unittest.TestCase):\n         error_log \u003d dispersion_report.get_error_log(\u0027test\u0027)\n         self.assertTrue(callable(error_log))\n \n-    def test_error_log_unmounted_device(self):\n-        \"\"\"Test error logging for unmounted devices (507 error)\"\"\"\n-        error_log \u003d dispersion_report.get_error_log(\u0027test_prefix\u0027)\n+    def test_unmounted_device_is_reported_once_across_loggers(self):\n+        \"\"\"Test unmounted devices are reported once per report run.\"\"\"\n+        container_error_log \u003d dispersion_report.get_error_log(\u0027container\u0027)\n+        object_error_log \u003d dispersion_report.get_error_log(\u0027object\u0027)\n \n         # Create a mock exception with http_status 507\n-        exc \u003d Mock()\n+        exc \u003d mock.Mock()\n         exc.http_status \u003d 507\n         exc.http_host \u003d \u0027192.168.1.1\u0027\n         exc.http_port \u003d 6000\n         exc.http_device \u003d \u0027sda\u0027\n \n-        # Just verify it doesn\u0027t raise an exception\n-        error_log(exc)\n-        # Check that the device was added to unmounted list\n-        self.assertIn(\u0027192.168.1.1:6000/sda\u0027, dispersion_report.unmounted)\n+        err \u003d StringIO()\n+        with mock.patch(\u0027swift.cli.dispersion_report.stderr\u0027, err):\n+            container_error_log(exc)\n+            object_error_log(exc)\n+\n+        self.assertEqual(\u0027ERROR: 192.168.1.1:6000/sda is unmounted --\u0027\n+                         \u0027 This will cause replicas designated for\u0027\n+                         \u0027 that device to be considered missing until\u0027\n+                         \u0027 resolved or the ring is updated.\\n\u0027,\n+                         err.getvalue())\n \n     def test_error_log_404_not_logged_when_debug_off(self):\n         \"\"\"Test that 404 errors don\u0027t add to notfound when debug is off\"\"\"\n         dispersion_report.debug \u003d False\n         error_log \u003d dispersion_report.get_error_log(\u0027test_prefix\u0027)\n \n-        exc \u003d Mock()\n+        exc \u003d mock.Mock()\n         exc.http_status \u003d 404\n         exc.http_host \u003d \u0027192.168.1.1\u0027\n         exc.http_port \u003d 6000\n```\n\nI\u0027d suggest we do something similar in \"404 when debug off\"","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        exc.http_port \u003d 6000"},{"line_number":48,"context_line":"        exc.http_device \u003d \u0027sda\u0027"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"        # Just verify it doesn\u0027t raise an exception"},{"line_number":51,"context_line":"        error_log(exc)"},{"line_number":52,"context_line":"        # Check that the device was added to unmounted list"},{"line_number":53,"context_line":"        self.assertIn(\u0027192.168.1.1:6000/sda\u0027, dispersion_report.unmounted)"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa1de787_867208c3","line":50,"in_reply_to":"3d0fcaf1_b9e9a9ff","updated":"2026-05-27 15:01:02.000000000","message":"Acknowledged","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":50,"context_line":"        # Just verify it doesn\u0027t raise an exception"},{"line_number":51,"context_line":"        error_log(exc)"},{"line_number":52,"context_line":"        # Check that the device was added to unmounted list"},{"line_number":53,"context_line":"        self.assertIn(\u0027192.168.1.1:6000/sda\u0027, dispersion_report.unmounted)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def test_error_log_404_not_logged_when_debug_off(self):"},{"line_number":56,"context_line":"        \"\"\"Test that 404 errors don\u0027t add to notfound when debug is off\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"15c2075c_b1ecf8af","line":53,"updated":"2026-05-15 17:35:00.000000000","message":"I think this assertion sends the wrong message - global state is bad taste, adding crap to module level lists is hardly something we want to \"enshrine\" as desirable by calling it out in tests - which should be oriented as much as possible around \"desirable behaviors we want to maintain\"\n\nI think \"the container \u0026 object report loggers should only print warnings about any specific unmounted device once\" was probably closer to the intent of the design had the original authors bothered to test it.","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":50,"context_line":"        # Just verify it doesn\u0027t raise an exception"},{"line_number":51,"context_line":"        error_log(exc)"},{"line_number":52,"context_line":"        # Check that the device was added to unmounted list"},{"line_number":53,"context_line":"        self.assertIn(\u0027192.168.1.1:6000/sda\u0027, dispersion_report.unmounted)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def test_error_log_404_not_logged_when_debug_off(self):"},{"line_number":56,"context_line":"        \"\"\"Test that 404 errors don\u0027t add to notfound when debug is off\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"545a9726_7a5f607e","line":53,"in_reply_to":"15c2075c_b1ecf8af","updated":"2026-05-27 15:01:02.000000000","message":"Acknowledged","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":84,"context_line":"        \"\"\"Test message for 1 partition missing 1 copy\"\"\""},{"line_number":85,"context_line":"        result \u003d dispersion_report.missing_string(1, 1, 3)"},{"line_number":86,"context_line":"        self.assertIn(\u00271 partition\u0027, result)"},{"line_number":87,"context_line":"        self.assertIn(\u0027missing 1 copy\u0027, result)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def test_multiple_partitions_single_copy_missing(self):"},{"line_number":90,"context_line":"        \"\"\"Test message for multiple partitions missing 1 copy each\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"839e954f_24f03a1b","line":87,"updated":"2026-05-15 17:35:00.000000000","message":"I use `assertIn` sometimes when I want to say something about logging side-effects; the main idea of the test would be \"make sure this method is correct (and also the logs sort of justify the edge-case/error/warning/info)\"\n\n... in this case the *entire behavior* of `missing_string` is to generate the helpful test about what happened.\n\nI think the test would look better with a literal:\n\n```\n@@ -83,8 +91,7 @@ class TestMissingString(unittest.TestCase):\n     def test_single_partition_single_copy_missing(self):\n         \"\"\"Test message for 1 partition missing 1 copy\"\"\"\n         result \u003d dispersion_report.missing_string(1, 1, 3)\n-        self.assertIn(\u00271 partition\u0027, result)\n-        self.assertIn(\u0027missing 1 copy\u0027, result)\n+        self.assertEqual(\u0027There was 1 partition missing 1 copy.\u0027, result)\n \n     def test_multiple_partitions_single_copy_missing(self):\n         \"\"\"Test message for multiple partitions missing 1 copy each\"\"\"\n```\n\n... same for remaining tests","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        \"\"\"Test message for 1 partition missing 1 copy\"\"\""},{"line_number":85,"context_line":"        result \u003d dispersion_report.missing_string(1, 1, 3)"},{"line_number":86,"context_line":"        self.assertIn(\u00271 partition\u0027, result)"},{"line_number":87,"context_line":"        self.assertIn(\u0027missing 1 copy\u0027, result)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def test_multiple_partitions_single_copy_missing(self):"},{"line_number":90,"context_line":"        \"\"\"Test message for multiple partitions missing 1 copy each\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ddd4d866_cca9cf22","line":87,"in_reply_to":"839e954f_24f03a1b","updated":"2026-05-27 15:01:02.000000000","message":"Acknowledged","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":110,"context_line":"        result \u003d dispersion_report.missing_string(1, 2, 3)"},{"line_number":111,"context_line":"        self.assertIn(\u0027!\u0027, result)"},{"line_number":112,"context_line":"        self.assertIn(\u00271 partition\u0027, result)"},{"line_number":113,"context_line":"        self.assertIn(\u0027missing 2 copies\u0027, result)"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"class TestGenerateReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"fdde6cc8_1b05e3c2","line":113,"updated":"2026-05-15 17:35:00.000000000","message":"this seems like it overlaps with the `(3, 2, 3)` case","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":110,"context_line":"        result \u003d dispersion_report.missing_string(1, 2, 3)"},{"line_number":111,"context_line":"        self.assertIn(\u0027!\u0027, result)"},{"line_number":112,"context_line":"        self.assertIn(\u00271 partition\u0027, result)"},{"line_number":113,"context_line":"        self.assertIn(\u0027missing 2 copies\u0027, result)"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"class TestGenerateReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"963da4bf_96d7e84f","line":113,"in_reply_to":"fdde6cc8_1b05e3c2","updated":"2026-05-27 15:01:02.000000000","message":"Done","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def test_generate_report_is_callable(self):"},{"line_number":128,"context_line":"        \"\"\"Test that generate_report is callable\"\"\""},{"line_number":129,"context_line":"        self.assertTrue(callable(dispersion_report.generate_report))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"class TestContainerDispersionReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ab9cee26_84206792","line":129,"updated":"2026-05-15 17:35:00.000000000","message":"this is a pretty week assertion on the most interesting bit\n\nIn all fairness it probably wouldn\u0027t be much of a problem in error_log and missing_string remained untested if we started to add some description of the behaviors we care about from generate report and produce some kind of infrastructure to describe and simulate the kinds of states that we expect the function to encounter.","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def test_generate_report_is_callable(self):"},{"line_number":128,"context_line":"        \"\"\"Test that generate_report is callable\"\"\""},{"line_number":129,"context_line":"        self.assertTrue(callable(dispersion_report.generate_report))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"class TestContainerDispersionReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"34c78b29_4866e53a","line":129,"in_reply_to":"ab9cee26_84206792","updated":"2026-05-27 15:01:02.000000000","message":"tested generate_report in detail!","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":142,"context_line":"    def tearDown(self):"},{"line_number":143,"context_line":"        \"\"\"Clean up after each test\"\"\""},{"line_number":144,"context_line":"        dispersion_report.unmounted \u003d []"},{"line_number":145,"context_line":"        dispersion_report.notfound \u003d []"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def test_container_report_is_callable(self):"},{"line_number":148,"context_line":"        \"\"\"Test that container_dispersion_report is callable\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ac85f8e4_e6f48db8","line":145,"updated":"2026-05-15 17:35:00.000000000","message":"manipulation of this set of module level globals appears far too often in this test module - please create a `reset_dispersion_module` helper function or probably a `BaseDispersionTest `","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":142,"context_line":"    def tearDown(self):"},{"line_number":143,"context_line":"        \"\"\"Clean up after each test\"\"\""},{"line_number":144,"context_line":"        dispersion_report.unmounted \u003d []"},{"line_number":145,"context_line":"        dispersion_report.notfound \u003d []"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def test_container_report_is_callable(self):"},{"line_number":148,"context_line":"        \"\"\"Test that container_dispersion_report is callable\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"f06889b3_c1ddfb25","line":145,"in_reply_to":"ac85f8e4_e6f48db8","updated":"2026-05-27 15:01:02.000000000","message":"used this:\ndef setUp(self):\n        \"\"\"Set up test fixtures\"\"\"\n        dispersion_report.json_output \u003d False\n\n    def tearDown(self):\n        \"\"\"Clean up after tests\"\"\"\n        dispersion_report.json_output \u003d False","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":147,"context_line":"    def test_container_report_is_callable(self):"},{"line_number":148,"context_line":"        \"\"\"Test that container_dispersion_report is callable\"\"\""},{"line_number":149,"context_line":"        self.assertTrue(callable("},{"line_number":150,"context_line":"            dispersion_report.container_dispersion_report))"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"class TestObjectDispersionReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"45caa523_f0a74ecc","line":150,"updated":"2026-05-15 17:35:00.000000000","message":"This probably wouldn\u0027t be necessary if our `TestGenerateReport` had some test infra that exercised the expected behavior when you pass these config options:\n\n```\n    container_report \u003d config_true_value(conf.get(\u0027container_report\u0027, \u0027yes\u0027))\n    object_report \u003d config_true_value(conf.get(\u0027object_report\u0027, \u0027yes\u0027))\n```\n\ne.g.\n\n```\ndef test_only_container():\n   conf \u003d {\u0027object_report\u0027: \u0027no\u0027}\n   with self.mock_report_functions():\n       dispersion_report.generate_report(conf)\n   self.assertTrue(self.mock_container_dispersion_report.called)\n   self.assertFalse(self.mock_object_dispersion_report.called)\n```","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":147,"context_line":"    def test_container_report_is_callable(self):"},{"line_number":148,"context_line":"        \"\"\"Test that container_dispersion_report is callable\"\"\""},{"line_number":149,"context_line":"        self.assertTrue(callable("},{"line_number":150,"context_line":"            dispersion_report.container_dispersion_report))"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"class TestObjectDispersionReport(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"8ad37b10_fca79e9a","line":150,"in_reply_to":"45caa523_f0a74ecc","updated":"2026-05-27 15:01:02.000000000","message":"Done","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cff733c9be9495922b65a5511034d67dc56baeb5","unresolved":true,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def test_object_report_is_callable(self):"},{"line_number":169,"context_line":"        \"\"\"Test that object_dispersion_report is callable\"\"\""},{"line_number":170,"context_line":"        self.assertTrue(callable(dispersion_report.object_dispersion_report))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"class TestMainFunction(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ed8b0362_20a6cb8f","line":170,"updated":"2026-05-15 17:35:00.000000000","message":"I worry there\u0027s actually a lot of duplicated code in `[object|container]_dispersion_report` that should probably be DRY\u0027d out\n\nI think it\u0027d be better to keep our testing focused on `generate_report` but eventually have them run with a mock `direct_client`/`SimpleClient`","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"68a3975f12447fea903484aa08ea4d1c711006fb","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def test_object_report_is_callable(self):"},{"line_number":169,"context_line":"        \"\"\"Test that object_dispersion_report is callable\"\"\""},{"line_number":170,"context_line":"        self.assertTrue(callable(dispersion_report.object_dispersion_report))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"class TestMainFunction(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1120acd0_7aafc2b5","line":170,"in_reply_to":"ed8b0362_20a6cb8f","updated":"2026-05-27 15:01:02.000000000","message":"added some relevant tests from 981400: add prom metrics to dispersion report | https://review.opendev.org/c/openstack/swift/+/981400","commit_id":"e7673bcb62863ba1bc5333f3ede5051791663872"}]}