)]}'
{"trove/guestagent/common/operating_system.py":[{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"c23fffdf7f81b5dd0251fa1bf481ebf12dcf26e5","unresolved":false,"context_lines":[{"line_number":102,"context_line":"    return found"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"def command_exists(command):"},{"line_number":106,"context_line":"    \"\"\"Check a given command exists."},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    :param command             Command to be checked."}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_3010344e","line":105,"updated":"2019-03-18 15:05:28.000000000","message":"We could make this method more usable and return the expanded path of the executable (find_executable). Since \"which\" command\u0027s logic is not very complex I would recommend implementing it in our operating_system module. Similarly to [1] but without support for Windows.\n\nThis is just a suggestion. The proposed solution is good enough. Your call whether to change it ot not.\n\n[1] https://github.com/python/cpython/blob/master/Lib/distutils/spawn.py#L169","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b20011c217f92a77055f860362577f429ab5f4c5","unresolved":false,"context_lines":[{"line_number":102,"context_line":"    return found"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"def command_exists(command):"},{"line_number":106,"context_line":"    \"\"\"Check a given command exists."},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    :param command             Command to be checked."}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_4a0c1294","line":105,"in_reply_to":"5fc1f717_3010344e","updated":"2019-03-22 10:45:11.000000000","message":"Following your suggestion in my new patch update I created a function called \u0027find_executable\u0027. Its effect is equivalent to \u0027which\u0027 command.","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"c23fffdf7f81b5dd0251fa1bf481ebf12dcf26e5","unresolved":false,"context_lines":[{"line_number":103,"context_line":""},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"def command_exists(command):"},{"line_number":106,"context_line":"    \"\"\"Check a given command exists."},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    :param command             Command to be checked."},{"line_number":109,"context_line":"    :type command              string"}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_2dd8dbec","line":106,"range":{"start_line":106,"start_character":13,"end_line":106,"end_character":14},"updated":"2019-03-18 15:05:28.000000000","message":"a -\u003e whether","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b20011c217f92a77055f860362577f429ab5f4c5","unresolved":false,"context_lines":[{"line_number":103,"context_line":""},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"def command_exists(command):"},{"line_number":106,"context_line":"    \"\"\"Check a given command exists."},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    :param command             Command to be checked."},{"line_number":109,"context_line":"    :type command              string"}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_ca1fa2b5","line":106,"range":{"start_line":106,"start_character":13,"end_line":106,"end_character":14},"in_reply_to":"5fc1f717_2dd8dbec","updated":"2019-03-22 10:45:11.000000000","message":"Done","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"c23fffdf7f81b5dd0251fa1bf481ebf12dcf26e5","unresolved":false,"context_lines":[{"line_number":110,"context_line":"    \"\"\""},{"line_number":111,"context_line":"    check_cmd \u003d ("},{"line_number":112,"context_line":"        \u0027which \"%s\" 2\u003e\u00261 \u003e /dev/null \u0026\u0026\u0027"},{"line_number":113,"context_line":"        \u0027 echo \"ok\" || echo \"not found\"\u0027 % command)"},{"line_number":114,"context_line":"    stdout, _ \u003d utils.execute_with_timeout(check_cmd, shell\u003dTrue)"},{"line_number":115,"context_line":"    return stdout \u003d\u003d \"ok\\n\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_cae49d3e","line":113,"range":{"start_line":113,"start_character":8,"end_line":113,"end_character":41},"updated":"2019-03-18 15:05:28.000000000","message":"Analogously to the previous method (\"exists\") let\u0027s echo a number - \"0\" or \"1\" - and return boolean cast of int:\n\n cmd \u003d \u0027test %s %s \u0026\u0026 echo 1 || echo 0\u0027 % (test_flag, path)\n stdout, _ \u003d utils.execute_with_timeout(cmd, ...)\n return bool(int(stdout))","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b20011c217f92a77055f860362577f429ab5f4c5","unresolved":false,"context_lines":[{"line_number":110,"context_line":"    \"\"\""},{"line_number":111,"context_line":"    check_cmd \u003d ("},{"line_number":112,"context_line":"        \u0027which \"%s\" 2\u003e\u00261 \u003e /dev/null \u0026\u0026\u0027"},{"line_number":113,"context_line":"        \u0027 echo \"ok\" || echo \"not found\"\u0027 % command)"},{"line_number":114,"context_line":"    stdout, _ \u003d utils.execute_with_timeout(check_cmd, shell\u003dTrue)"},{"line_number":115,"context_line":"    return stdout \u003d\u003d \"ok\\n\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_8a775a09","line":113,"range":{"start_line":113,"start_character":8,"end_line":113,"end_character":41},"in_reply_to":"5fc1f717_cae49d3e","updated":"2019-03-22 10:45:11.000000000","message":"Done","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"06ae0fa30d08ef709e4454d55b873125dff88c53","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    :type path                 string"},{"line_number":114,"context_line":"    \"\"\""},{"line_number":115,"context_line":"    dirs \u003d (os.environ.get(\u0027PATH\u0027, os.defpath).split(os.pathsep)"},{"line_number":116,"context_line":"            if path is None else path.split(os.pathsep))"},{"line_number":117,"context_line":"    paths \u003d map(os.path.join,"},{"line_number":118,"context_line":"                dirs,"},{"line_number":119,"context_line":"                (executable for _ in range(len(dirs))))"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_f2680bbb","line":116,"updated":"2019-03-22 10:06:42.000000000","message":"We can simplify the above code by decomposing the statement as follows:\n\n if path is None:\n     path \u003d os.environ.get(\u0027PATH\u0027, os.defpath)\n dirs \u003d path.split(os.pathsep)","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b3b135d3bba2bcf0934372cfbf6473f576b79317","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    :type path                 string"},{"line_number":114,"context_line":"    \"\"\""},{"line_number":115,"context_line":"    dirs \u003d (os.environ.get(\u0027PATH\u0027, os.defpath).split(os.pathsep)"},{"line_number":116,"context_line":"            if path is None else path.split(os.pathsep))"},{"line_number":117,"context_line":"    paths \u003d map(os.path.join,"},{"line_number":118,"context_line":"                dirs,"},{"line_number":119,"context_line":"                (executable for _ in range(len(dirs))))"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_5056852c","line":116,"in_reply_to":"5fc1f717_f2680bbb","updated":"2019-03-25 09:06:18.000000000","message":"Done","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"06ae0fa30d08ef709e4454d55b873125dff88c53","unresolved":false,"context_lines":[{"line_number":114,"context_line":"    \"\"\""},{"line_number":115,"context_line":"    dirs \u003d (os.environ.get(\u0027PATH\u0027, os.defpath).split(os.pathsep)"},{"line_number":116,"context_line":"            if path is None else path.split(os.pathsep))"},{"line_number":117,"context_line":"    paths \u003d map(os.path.join,"},{"line_number":118,"context_line":"                dirs,"},{"line_number":119,"context_line":"                (executable for _ in range(len(dirs))))"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def is_executable(path):"},{"line_number":122,"context_line":"        return os.path.isfile(path) and os.access(path, os.X_OK)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_ff0c5443","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":55},"updated":"2019-03-22 10:06:42.000000000","message":"Maps are great for simple one-liners, but unfortunately they degrade the readability. In this case, I would rather go with traditional for loop, join each dir with the executable in a clear statement, and check the executable existance in-the-fly - return immadiately.\n\nAlso, generating a list of the same \"executable\" elements does not seem to by a good practice. Don\u0027t be afraid of splitting the code into multiple lines. In the community code, choose expressiveness and clarity over code compaction.","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b3b135d3bba2bcf0934372cfbf6473f576b79317","unresolved":false,"context_lines":[{"line_number":114,"context_line":"    \"\"\""},{"line_number":115,"context_line":"    dirs \u003d (os.environ.get(\u0027PATH\u0027, os.defpath).split(os.pathsep)"},{"line_number":116,"context_line":"            if path is None else path.split(os.pathsep))"},{"line_number":117,"context_line":"    paths \u003d map(os.path.join,"},{"line_number":118,"context_line":"                dirs,"},{"line_number":119,"context_line":"                (executable for _ in range(len(dirs))))"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def is_executable(path):"},{"line_number":122,"context_line":"        return os.path.isfile(path) and os.access(path, os.X_OK)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_7053813a","line":119,"range":{"start_line":117,"start_character":0,"end_line":119,"end_character":55},"in_reply_to":"5fc1f717_ff0c5443","updated":"2019-03-25 09:06:18.000000000","message":"Done","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"06ae0fa30d08ef709e4454d55b873125dff88c53","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    def is_executable(path):"},{"line_number":122,"context_line":"        return os.path.isfile(path) and os.access(path, os.X_OK)"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return next((path for path in paths if is_executable(path)), None)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"def _read_file_as_root(path, open_flag, convert_func):"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_52a79f0c","line":124,"updated":"2019-03-22 10:06:42.000000000","message":"Let\u0027s return the executable path earlier in the for-loop, as proposed above, instead of using an iterator. We can save several bytes of memory by not generating a full list of possible executable paths :)","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b3b135d3bba2bcf0934372cfbf6473f576b79317","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    def is_executable(path):"},{"line_number":122,"context_line":"        return os.path.isfile(path) and os.access(path, os.X_OK)"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return next((path for path in paths if is_executable(path)), None)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"def _read_file_as_root(path, open_flag, convert_func):"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_30373902","line":124,"in_reply_to":"5fc1f717_52a79f0c","updated":"2019-03-25 09:06:18.000000000","message":"Done","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"77f1874208eb479415b7d61716364f3031b718eb","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        path \u003d os.environ.get(\u0027PATH\u0027, os.defpath)"},{"line_number":117,"context_line":"    dirs \u003d path.split(os.pathsep)"},{"line_number":118,"context_line":"    for directory in dirs:"},{"line_number":119,"context_line":"        full_path \u003d os.path.join(directory, executable)"},{"line_number":120,"context_line":"        if os.path.isfile(full_path) and os.access(full_path, os.X_OK):"},{"line_number":121,"context_line":"            return full_path"},{"line_number":122,"context_line":"    return None"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_b4d69df1","line":119,"range":{"start_line":119,"start_character":8,"end_line":119,"end_character":17},"updated":"2019-03-26 09:06:43.000000000","message":"Consider rename: exec_path","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"}],"trove/guestagent/datastore/experimental/mariadb/service.py":[{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"ff4afb5f9fcba4c2ad2671cecc1cad14e3d95822","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                % result[\u0027service\u0027])"},{"line_number":51,"context_line":"        elif result[\u0027type\u0027] \u003d\u003d \u0027systemd\u0027:"},{"line_number":52,"context_line":"            # TODO(mwj 2016/01/28): determine RHEL start for MariaDB Cluster"},{"line_number":53,"context_line":"            if operating_system.exists(\u0027/usr/bin/galera_new_cluster\u0027):"},{"line_number":54,"context_line":"                result[\u0027cmd_bootstrap_galera_cluster\u0027] \u003d ("},{"line_number":55,"context_line":"                    \"sudo /usr/bin/galera_new_cluster\")"},{"line_number":56,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_4a3ba438","line":53,"range":{"start_line":53,"start_character":40,"end_line":53,"end_character":67},"updated":"2019-03-04 11:43:22.000000000","message":"Please, ensure that this path is valid for supported OS distros, in particular Ubuntu, RHEL and CentOS. Change [1] in MariaDB repository introducing galera_new_cluster script might be helpful for determining the differences - you can include it in commit message as well.\n\n[1] https://github.com/MariaDB/server/commit/a2c3549404cbdc55aaaa8537739853d7369aa79a#diff-69c2c819ad6db126245fc3aeafd68077","commit_id":"d12889e7aa70e266c3e0cc1a39bf2465cf0a8e42"}],"trove/tests/unittests/guestagent/test_operating_system.py":[{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"c23fffdf7f81b5dd0251fa1bf481ebf12dcf26e5","unresolved":false,"context_lines":[{"line_number":991,"context_line":"            operating_system.command_exists(\"non existing command\"))"},{"line_number":992,"context_line":"        self.assertTrue("},{"line_number":993,"context_line":"            operating_system.command_exists(\"which\"))"},{"line_number":994,"context_line":""},{"line_number":995,"context_line":"    def test_exists(self):"},{"line_number":996,"context_line":"        self.assertFalse("},{"line_number":997,"context_line":"            operating_system.exists(tempfile.gettempdir(), is_directory\u003dFalse))"}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_4a398def","line":994,"updated":"2019-03-18 15:05:28.000000000","message":"Add assertion for command call. You can use \"_assert_execute_call\" helper method.","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"b20011c217f92a77055f860362577f429ab5f4c5","unresolved":false,"context_lines":[{"line_number":991,"context_line":"            operating_system.command_exists(\"non existing command\"))"},{"line_number":992,"context_line":"        self.assertTrue("},{"line_number":993,"context_line":"            operating_system.command_exists(\"which\"))"},{"line_number":994,"context_line":""},{"line_number":995,"context_line":"    def test_exists(self):"},{"line_number":996,"context_line":"        self.assertFalse("},{"line_number":997,"context_line":"            operating_system.exists(tempfile.gettempdir(), is_directory\u003dFalse))"}],"source_content_type":"text/x-python","patch_set":3,"id":"5fc1f717_8aee7a89","line":994,"in_reply_to":"5fc1f717_4a398def","updated":"2019-03-22 10:45:11.000000000","message":"As in my patch update method \u0027command_exists\u0027 has been replaced and the new one doesn\u0027t make use of shell commands, these assertions are no longer necessary.","commit_id":"f4cfffd05aa7be761b3cf337f5bbb2eaa9ed98ea"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"06ae0fa30d08ef709e4454d55b873125dff88c53","unresolved":false,"context_lines":[{"line_number":985,"context_line":"            \u0027path\u0027, _unknown_kw\u003d0)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        self.assertIsNone("},{"line_number":989,"context_line":"            operating_system.find_executable(\"non_existing_command\"))"},{"line_number":990,"context_line":"        self.assertIsNone("},{"line_number":991,"context_line":"            operating_system.find_executable(\"non existing command\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_72a09ba6","line":988,"updated":"2019-03-22 10:06:42.000000000","message":"Executing bash commands on a bare operating system is not exactly what unit tests are intended for. Unit tests should run in isolation from other methods, classes and processes. The complete integration of method call should be tested by integration tests. Given above, we should mock/stub dependent method executions to provide appropriate isolation.\n\nFor example, we could do as follows:\n1. Stub the \"os.environ.get(\u0027PATH\u0027, os.defpath)\" execution, replace return value with arbitrary path: \"/usr/bin:/usr/local/bin\"\n2. Stub \"os.path.isfile(path)\" executions to \"simulate\" that the executable exists.\n\nYou can read more about Python stubs and mocks in [1]. Also, look at naighbour code for more examples. Search for \"patch\" phrase.\n\nMoreover, since \"find_executable\" method has optional \"path\" argument we should test two scenarios: with and without \"path\" parameter provided. To reflect this scenarios, let\u0027s add separate testing methods for each of them, e.g., test_find_executable_with_path and test_find_executable_without_path.\n\n[1] https://docs.python.org/3/library/unittest.mock.html","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"34f325505df878de7baee27b38757330e5f9a8d8","unresolved":false,"context_lines":[{"line_number":985,"context_line":"            \u0027path\u0027, _unknown_kw\u003d0)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        self.assertIsNone("},{"line_number":989,"context_line":"            operating_system.find_executable(\"non_existing_command\"))"},{"line_number":990,"context_line":"        self.assertIsNone("},{"line_number":991,"context_line":"            operating_system.find_executable(\"non existing command\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_f7f75d8a","line":988,"in_reply_to":"5fc1f717_72a09ba6","updated":"2019-03-25 14:25:37.000000000","message":"Done","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"06ae0fa30d08ef709e4454d55b873125dff88c53","unresolved":false,"context_lines":[{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        self.assertIsNone("},{"line_number":989,"context_line":"            operating_system.find_executable(\"non_existing_command\"))"},{"line_number":990,"context_line":"        self.assertIsNone("},{"line_number":991,"context_line":"            operating_system.find_executable(\"non existing command\"))"},{"line_number":992,"context_line":"        possible_results \u003d [\"/bin/sh\", \"/usr/bin/sh\"]"},{"line_number":993,"context_line":"        actual_result \u003d operating_system.find_executable(\"sh\")"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_d28b8f65","line":990,"updated":"2019-03-22 10:06:42.000000000","message":"This line is a duplicate of the previous one.","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"34f325505df878de7baee27b38757330e5f9a8d8","unresolved":false,"context_lines":[{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        self.assertIsNone("},{"line_number":989,"context_line":"            operating_system.find_executable(\"non_existing_command\"))"},{"line_number":990,"context_line":"        self.assertIsNone("},{"line_number":991,"context_line":"            operating_system.find_executable(\"non existing command\"))"},{"line_number":992,"context_line":"        possible_results \u003d [\"/bin/sh\", \"/usr/bin/sh\"]"},{"line_number":993,"context_line":"        actual_result \u003d operating_system.find_executable(\"sh\")"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_b7ed55b8","line":990,"in_reply_to":"5fc1f717_d28b8f65","updated":"2019-03-25 14:25:37.000000000","message":"Done","commit_id":"3f4c3d24155855dcdfe6c27fc6e17c1e3052d6cd"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"77f1874208eb479415b7d61716364f3031b718eb","unresolved":false,"context_lines":[{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        command \u003d \"command\""},{"line_number":989,"context_line":"        # Each tuple represents return values of os.path.isfile, os.access and"},{"line_number":990,"context_line":"        # indicator describing if the result -f find_executable should be None"},{"line_number":991,"context_line":"        param_list \u003d ["},{"line_number":992,"context_line":"            (True, True, False),"},{"line_number":993,"context_line":"            (True, False, True),"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_af188475","line":990,"range":{"start_line":989,"start_character":0,"end_line":990,"end_character":78},"updated":"2019-03-26 09:06:43.000000000","message":"Such comments should not be necessary. Python is an expressive language. Let\u0027s use it!\n\nInstead of a raw list of tuples:\n\n param_list \u003d [\n     (True, True, False),\n     (True, False, True),\n     (False, True, True),\n     (False, False, True),\n ]\n\n... we could execute each scenario in a separate assertion statement:\n\n self._assert_find_executable(\n     command\u003dcommand,\n     path\u003dNone,\n     is_file\u003dTrue,\n     is_executable\u003dTrue,\n     expected_return_value\u003d\u0027/usr/bin/command\u0027)\n\n self._assert_find_executable(\n     command\u003dcommand,\n     path\u003dNone,\n     is_file\u003dFalse,\n     is_executable\u003dFalse,\n     expected_return_value\u003dNone)\n\nMore lines is OK, especially in test cases. First, we get better readability. Second, such tests are easier to debug - instead of getting a trace reference to a line such as:\n\n self._assert_find_executable(command, None, isfile, access, should_be_none)\n\n... which does not say what were the call arguments, we get:\n\n self._assert_find_executable(\n     command\u003dcommand,\n     path\u003dNone,\n     is_file\u003dFalse,\n     is_executable\u003dFalse,\n     expected_return_value\u003dNone)\n\n.. which says exactly what the execution context is.","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"5fba24e6536b2656df8d5b39fd2d34289dc59063","unresolved":false,"context_lines":[{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def test_find_executable(self):"},{"line_number":988,"context_line":"        command \u003d \"command\""},{"line_number":989,"context_line":"        # Each tuple represents return values of os.path.isfile, os.access and"},{"line_number":990,"context_line":"        # indicator describing if the result -f find_executable should be None"},{"line_number":991,"context_line":"        param_list \u003d ["},{"line_number":992,"context_line":"            (True, True, False),"},{"line_number":993,"context_line":"            (True, False, True),"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_6f0b7b29","line":990,"range":{"start_line":989,"start_character":0,"end_line":990,"end_character":78},"in_reply_to":"5fc1f717_af188475","updated":"2019-03-26 10:56:25.000000000","message":"Done","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"77f1874208eb479415b7d61716364f3031b718eb","unresolved":false,"context_lines":[{"line_number":998,"context_line":"        for isfile, access, should_be_none in param_list:"},{"line_number":999,"context_line":"            self._assert_find_executable(command, None, isfile,"},{"line_number":1000,"context_line":"                                         access, should_be_none)"},{"line_number":1001,"context_line":"        # With path specified"},{"line_number":1002,"context_line":"        for isfile, access, should_be_none in param_list:"},{"line_number":1003,"context_line":"            self._assert_find_executable(command, \"/path\", isfile,"},{"line_number":1004,"context_line":"                                         access, should_be_none)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"    @patch.object(os.environ, \u0027get\u0027, return_value\u003d\"/usr/bin\")"},{"line_number":1007,"context_line":"    def _assert_find_executable(self, command, path, isfile, access,"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_8f3ca893","line":1004,"range":{"start_line":1001,"start_character":0,"end_line":1004,"end_character":64},"updated":"2019-03-26 09:06:43.000000000","message":"As I suggested in the previous review, a good practice is to split method test into multiple testing methods. In this case, we could have separate methods for testing \"find_executable\" with and without \"path\" argument, e.g., \"test_find_executable_with_path\" and \"test_find_executable_without_path\".","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"5fba24e6536b2656df8d5b39fd2d34289dc59063","unresolved":false,"context_lines":[{"line_number":998,"context_line":"        for isfile, access, should_be_none in param_list:"},{"line_number":999,"context_line":"            self._assert_find_executable(command, None, isfile,"},{"line_number":1000,"context_line":"                                         access, should_be_none)"},{"line_number":1001,"context_line":"        # With path specified"},{"line_number":1002,"context_line":"        for isfile, access, should_be_none in param_list:"},{"line_number":1003,"context_line":"            self._assert_find_executable(command, \"/path\", isfile,"},{"line_number":1004,"context_line":"                                         access, should_be_none)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"    @patch.object(os.environ, \u0027get\u0027, return_value\u003d\"/usr/bin\")"},{"line_number":1007,"context_line":"    def _assert_find_executable(self, command, path, isfile, access,"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_52d0a81f","line":1004,"range":{"start_line":1001,"start_character":0,"end_line":1004,"end_character":64},"in_reply_to":"5fc1f717_8f3ca893","updated":"2019-03-26 10:56:25.000000000","message":"Done","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"77f1874208eb479415b7d61716364f3031b718eb","unresolved":false,"context_lines":[{"line_number":1007,"context_line":"    def _assert_find_executable(self, command, path, isfile, access,"},{"line_number":1008,"context_line":"                                should_be_none, mock_environ):"},{"line_number":1009,"context_line":"        should_call_environ_get \u003d True if path is None else False"},{"line_number":1010,"context_line":"        if path is None:"},{"line_number":1011,"context_line":"            full_path \u003d os.path.join(mock_environ.return_value, command)"},{"line_number":1012,"context_line":"        else:"},{"line_number":1013,"context_line":"            full_path \u003d os.path.join(path, command)"},{"line_number":1014,"context_line":"        with patch.object(os.path, \u0027isfile\u0027, return_value\u003disfile):"},{"line_number":1015,"context_line":"            with patch.object(os, \u0027access\u0027, return_value\u003daccess):"},{"line_number":1016,"context_line":"                actual_result \u003d operating_system.find_executable(command,"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_2f7a5476","line":1013,"range":{"start_line":1010,"start_character":0,"end_line":1013,"end_character":51},"updated":"2019-03-26 09:06:43.000000000","message":"Avoid reimplementing the logic under tests - error-prone, degrades readability. Instead, hardcode the expected return values. Note:\n\n expected_return_value\u003d\u0027/usr/bin/command\u0027\n\n... in the previous comment.","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"5fba24e6536b2656df8d5b39fd2d34289dc59063","unresolved":false,"context_lines":[{"line_number":1007,"context_line":"    def _assert_find_executable(self, command, path, isfile, access,"},{"line_number":1008,"context_line":"                                should_be_none, mock_environ):"},{"line_number":1009,"context_line":"        should_call_environ_get \u003d True if path is None else False"},{"line_number":1010,"context_line":"        if path is None:"},{"line_number":1011,"context_line":"            full_path \u003d os.path.join(mock_environ.return_value, command)"},{"line_number":1012,"context_line":"        else:"},{"line_number":1013,"context_line":"            full_path \u003d os.path.join(path, command)"},{"line_number":1014,"context_line":"        with patch.object(os.path, \u0027isfile\u0027, return_value\u003disfile):"},{"line_number":1015,"context_line":"            with patch.object(os, \u0027access\u0027, return_value\u003daccess):"},{"line_number":1016,"context_line":"                actual_result \u003d operating_system.find_executable(command,"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_32987c73","line":1013,"range":{"start_line":1010,"start_character":0,"end_line":1013,"end_character":51},"in_reply_to":"5fc1f717_2f7a5476","updated":"2019-03-26 10:56:25.000000000","message":"Done","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"77f1874208eb479415b7d61716364f3031b718eb","unresolved":false,"context_lines":[{"line_number":1019,"context_line":"            self.assertIsNone(actual_result)"},{"line_number":1020,"context_line":"        else:"},{"line_number":1021,"context_line":"            self.assertEqual(full_path, actual_result)"},{"line_number":1022,"context_line":"        if should_call_environ_get:"},{"line_number":1023,"context_line":"            mock_environ.assert_called_once()"},{"line_number":1024,"context_line":"        else:"},{"line_number":1025,"context_line":"            mock_environ.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_2f0b34d5","line":1022,"range":{"start_line":1022,"start_character":11,"end_line":1022,"end_character":34},"updated":"2019-03-26 09:06:43.000000000","message":"I think we can get rid of \"should_call_environ_get\" variable by specifying a simple condition:\n\n if path:\n     mock_environ.assert_called_once()\n else:\n     ...","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"5fba24e6536b2656df8d5b39fd2d34289dc59063","unresolved":false,"context_lines":[{"line_number":1019,"context_line":"            self.assertIsNone(actual_result)"},{"line_number":1020,"context_line":"        else:"},{"line_number":1021,"context_line":"            self.assertEqual(full_path, actual_result)"},{"line_number":1022,"context_line":"        if should_call_environ_get:"},{"line_number":1023,"context_line":"            mock_environ.assert_called_once()"},{"line_number":1024,"context_line":"        else:"},{"line_number":1025,"context_line":"            mock_environ.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5fc1f717_129b0073","line":1022,"range":{"start_line":1022,"start_character":11,"end_line":1022,"end_character":34},"in_reply_to":"5fc1f717_2f0b34d5","updated":"2019-03-26 10:56:25.000000000","message":"Done","commit_id":"47a84c5ab0f7819eae22132033f908e5ec6f0db7"},{"author":{"_account_id":28695,"name":"Bartosz Zurkowski","email":"b.zurkowski@samsung.com","username":"b.zurkowski"},"change_message_id":"3b9aeb3ecdff69dc8a51cfcafa6e8634518ee7ae","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _assert_find_executable(self, command, path, isfile,"},{"line_number":1035,"context_line":"                                access, expected_return_value):"},{"line_number":1036,"context_line":"        with patch.object(os.environ, \u0027get\u0027,"},{"line_number":1037,"context_line":"                          return_value\u003d\"/usr/bin\") as mock_environ:"},{"line_number":1038,"context_line":"            with patch.object(os.path, \u0027isfile\u0027, return_value\u003disfile):"},{"line_number":1039,"context_line":"                with patch.object(os, \u0027access\u0027, return_value\u003daccess):"},{"line_number":1040,"context_line":"                    actual_result \u003d operating_system.find_executable(command,"},{"line_number":1041,"context_line":"                                                                     path)"},{"line_number":1042,"context_line":"            self.assertEqual(expected_return_value, actual_result)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5fc1f717_e6321610","line":1039,"range":{"start_line":1036,"start_character":0,"end_line":1039,"end_character":69},"updated":"2019-03-27 13:52:30.000000000","message":"Too much nesting. We could try to initialize mocks via method decorators and assign return value via mock setter method [1]:\n\n with patch(\u0027__main__.Foo.foo\u0027, new_callable\u003dPropertyMock) as mock_foo:\n     mock_foo.return_value \u003d \u0027mockity-mock\u0027\n     this_foo \u003d Foo()\n     print(this_foo.foo)\n     this_foo.foo \u003d 6\n\nSomething similar to this:\n\n @patch.object(...)\n def test_find_executable(self, some_mock, arg1):\n     some_mock.return_value \u003d arg1\n     ...\n\n\n[1] https://docs.python.org/3/library/unittest.mock.html#unittest.mock.PropertyMock","commit_id":"df899f9dc3051df47934a0f1ddb94b42178916b0"},{"author":{"_account_id":29995,"name":"Kasper Hasior","email":"k.hasior@partner.samsung.com","username":"k.hasior"},"change_message_id":"92835df80ce37f7fc2f1c5a9c4377aca1cdafc63","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _assert_find_executable(self, command, path, isfile,"},{"line_number":1035,"context_line":"                                access, expected_return_value):"},{"line_number":1036,"context_line":"        with patch.object(os.environ, \u0027get\u0027,"},{"line_number":1037,"context_line":"                          return_value\u003d\"/usr/bin\") as mock_environ:"},{"line_number":1038,"context_line":"            with patch.object(os.path, \u0027isfile\u0027, return_value\u003disfile):"},{"line_number":1039,"context_line":"                with patch.object(os, \u0027access\u0027, return_value\u003daccess):"},{"line_number":1040,"context_line":"                    actual_result \u003d operating_system.find_executable(command,"},{"line_number":1041,"context_line":"                                                                     path)"},{"line_number":1042,"context_line":"            self.assertEqual(expected_return_value, actual_result)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5fc1f717_9ff00c93","line":1039,"range":{"start_line":1036,"start_character":0,"end_line":1039,"end_character":69},"in_reply_to":"5fc1f717_e6321610","updated":"2019-03-29 10:29:32.000000000","message":"Using patch decorators makes it impossible to call decorated function with positional arguments so I have created a function _delegate_assert_find_executable which acts as a middleman.","commit_id":"df899f9dc3051df47934a0f1ddb94b42178916b0"}]}