)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"99620bb16ac3877fcbb1d6929075bb287d9afe15","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"bd259eb7_da04e2d0","updated":"2022-10-06 13:21:28.000000000","message":"recheck unrelated? timeout","commit_id":"6d12766dbc08b55861ab6a05c123e6969a53703b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2666a72f10cee90563de628c4ced5ce60733ec30","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b7ad820f_df8d76df","updated":"2022-10-07 13:34:00.000000000","message":"recheck neutron fix landed","commit_id":"265b78b51b15eaf4cc76075613bed1330e4fa0ae"},{"author":{"_account_id":30742,"name":"Soniya Murlidhar Vyas","email":"svyas@redhat.com","username":"svyas"},"change_message_id":"8365354fd80f8bd94c89b7fcb1ee9a6d03adb44b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"cec7e3e9_8cfa4bd6","updated":"2022-10-07 16:36:52.000000000","message":"We are repeating this code in the above tests as well, IMHO we dont require this..since we are anyhow testing in the above tests as well[1].\n\n[1]https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"759dad334203b294ae3aae8e7640d7d2b5876340","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"2260b118_afde1248","in_reply_to":"05915ae3_6399e986","updated":"2022-10-12 17:41:16.000000000","message":"Let\u0027s do any kind of refactoring in a separate patch, it\u0027s always a good idea and I\u0027d even say a good practice not to mix refactoring changes with the changes which add a new logic/feature/test etc.\n\nI understand why the code in this file may look like it contains repetitive code, but at the same time I personally like the structure of the file and I find it easy to read - based on the length of the file as well as the length of the repeated code, this is the situation where we need to weigh \u0027easy to ready and understand\u0027 against just a few repeated lines ... and I personally vote \u0027easy to read and understand\u0027 in this situation. \n\nThat said, we could wrap (in a separate patch) the following lines:\n```\n        snapshot_name \u003d data_utils.rand_name(\u0027test-snap\u0027)\n        image \u003d self.create_image_from_server(server[\u0027id\u0027],\n                                              name\u003dsnapshot_name,\n                                              wait_until\u003d\u0027ACTIVE\u0027,\n                                              wait_for_server\u003dFalse)\n        self.addCleanup(self.client.delete_image, image[\u0027id\u0027])\n```\ninto a helper method but we really need to ask ourselves whether its worth it - we\u0027ll save 10 maybe 15 lines of code but we\u0027ll gain a new method we need to trace when we debug the tests ... I\u0027m really not sure if that\u0027s worth it in this particular case","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"9afd46948fbab1f1250203c1ef53c64eca472205","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"24f6feb3_105712bc","in_reply_to":"16998596_54e01842","updated":"2022-10-12 17:53:05.000000000","message":"I agree on the readability part, if refactoring does not give much value then it make sense to keep it more readable.\n\nAnyways that is separate discussion and patch we can discuss the refactoring if needed and we should not block this test to land. Test running fine in normal as well as in ceph job.\n\nApproving it, will discuss about refactoring things with Sonia later once she is online in IRC or in her patch.","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2e9bc74aebfbfc99922af599212596226f1f663f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"16998596_54e01842","in_reply_to":"2260b118_afde1248","updated":"2022-10-12 17:50:26.000000000","message":"\u003e That said, we could wrap (in a separate patch) the following lines:\n\u003e ```\n\u003e         snapshot_name \u003d data_utils.rand_name(\u0027test-snap\u0027)\n\u003e         image \u003d self.create_image_from_server(server[\u0027id\u0027],\n\u003e                                               name\u003dsnapshot_name,\n\u003e                                               wait_until\u003d\u0027ACTIVE\u0027,\n\u003e                                               wait_for_server\u003dFalse)\n\u003e         self.addCleanup(self.client.delete_image, image[\u0027id\u0027])\n\u003e ```\n\u003e into a helper method but we really need to ask ourselves whether its worth it - we\u0027ll save 10 maybe 15 lines of code but we\u0027ll gain a new method we need to trace when we debug the tests ... I\u0027m really not sure if that\u0027s worth it in this particular case\n\nAgree. In your example case you also need to either assert or return the name so that the tests that do that keep that coverage. If I have to scroll away from a ten-line test to determine if a helper is asserting something on behalf of the test, that\u0027s a loss of readability that outweighs a little code reuse, IMHO.","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":30742,"name":"Soniya Murlidhar Vyas","email":"svyas@redhat.com","username":"svyas"},"change_message_id":"07f004a25741fe4a92a8c14dd71a278fe985c1c9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"82610d33_62ec9dfe","in_reply_to":"2260b118_afde1248","updated":"2022-10-12 17:51:30.000000000","message":"yupp, adding a helper here will save 10 lines but then one more method to debug, in such a case, I revote.","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":30742,"name":"Soniya Murlidhar Vyas","email":"svyas@redhat.com","username":"svyas"},"change_message_id":"e2f846cd766f9514ff673310b981e00aab211018","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"a253b22b_3d3b2e26","in_reply_to":"59f8464a_91b421ee","updated":"2022-10-11 08:39:41.000000000","message":"So, if we look at the source code of test_images.py[1]. The following steps are repeated at L#62[2], L#87[3], L#106[4], L#125[5] as follows:- \n\n1. creating test_server\n2. taking a snapshot of the test_server\n\nI agree the scenario tested in the patch is valid but we also need to take into account that we are repeating the code which has already been repeated multiple times.\n\nCan we find a solution for this? \nSomething like..\n\n1. create_snapshot() -\u003e an internal method which can be used by all the tests\n2. use above method here[6]\n\n[1] https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py\n[2] https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py#L62\n[3] https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py#L87\n[4] https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py#L106\n[5] https://opendev.org/openstack/tempest/src/branch/master/tempest/api/compute/images/test_images.py#L125\n[6] https://review.opendev.org/c/openstack/tempest/+/860502/7/tempest/api/compute/images/test_images.py#133","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5434f83b26357f087ace3135edc8a37a19d7e837","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"80eb9d10_f1a4599c","in_reply_to":"770ef3cb_b53ad471","updated":"2022-10-11 14:48:02.000000000","message":"And I guess I should say: It seems to me like the existing tests already re-use plenty of the same code. We already have helpers for create_test_server, create_image_from_server, etc. We could combine some of the waiting stuff with the creation, but in general, this seems to be on par with the level of granularity I see elsewhere in our tests.\n\nSo maybe the best thing would be to have you put a patch up with some proposed refactoring we could discuss? I could either rebase this on that, if so, or we could put that at the end of this, separate from the adding of the new test scenario?","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":30742,"name":"Soniya Murlidhar Vyas","email":"svyas@redhat.com","username":"svyas"},"change_message_id":"fc0636a8ce1cd5faeee120d0621f0ce8f00cba5b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"05915ae3_6399e986","in_reply_to":"80eb9d10_f1a4599c","updated":"2022-10-12 10:34:30.000000000","message":"I agree you need to really make a effort of opening six tabs here but in that case you can help yourself by mapping these line numbers to file itself. I have already hinted the lines 😊.\n\n \u003e\u003e Are you saying I need to refactor all the tests before I add mine?\n\nExactly.\n\n \u003e\u003e It seems to me like the existing tests already re-use plenty of the same code.\n \u003e\u003e We already have helpers for create_test_server, create_image_from_server, etc.\n \u003e\u003e We could combine some of the waiting stuff with the creation, but in general,\n \u003e\u003e this seems to be on par with the level of granularity I see elsewhere in our \n \u003e\u003e tests.\n\nSame goes with all of them..we need a refactoring there as well.\n\n \u003e\u003e So maybe the best thing would be to have you put a patch up with some proposed refactoring we could discuss?\n \nSure, I can add a patch refactoring the earlier tests. And on that patch you can rebase. So till then, will you be okay to mark this patch as WIP..it would be easy for reviewers?","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b51198c0d2ea9c4285c0822d15dcdeddbbf594b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"770ef3cb_b53ad471","in_reply_to":"a253b22b_3d3b2e26","updated":"2022-10-11 14:44:28.000000000","message":"It would be a lot more helpful if you could comment on the actual lines of the file instead of github links. Otherwise I have to open six tabs 😊\n\nThat said, I don\u0027t think this makes the situation worse than it is, and in fact mirrors the behavior of the tests above it right? Refactoring the tests might be a good idea, but I\u0027d rather have that be in a separate patch than this one that just adds the test. That makes the history cleaner and future problem determination easier to point to the refactor or the test.\n\nAre you saying I need to refactor all the tests before I add mine? How about I tack a patch on the end of this that tries to DRY them up some after the fact?","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"73fe523f208375667260acf97100c99f9243ea90","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"59f8464a_91b421ee","in_reply_to":"cec7e3e9_8cfa4bd6","updated":"2022-10-10 13:54:05.000000000","message":"Where? Can you point me to a line number?","commit_id":"fe00e58d13e27d31de0ea2ebfa06b8c216475fae"},{"author":{"_account_id":30742,"name":"Soniya Murlidhar Vyas","email":"svyas@redhat.com","username":"svyas"},"change_message_id":"e2f846cd766f9514ff673310b981e00aab211018","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"e9abf032_44daff9c","updated":"2022-10-11 08:39:41.000000000","message":"Code redundancy is the main concern here. Otherwise scenario seems okay.","commit_id":"6714b65a210c5851591dfd5dfe9ad1cbf1fa49e9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"73fe523f208375667260acf97100c99f9243ea90","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"ac40caad_3fd666b1","updated":"2022-10-10 13:54:05.000000000","message":"Oops, apparently I never committed this.","commit_id":"6714b65a210c5851591dfd5dfe9ad1cbf1fa49e9"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"759dad334203b294ae3aae8e7640d7d2b5876340","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"99796a4d_9284c4c1","updated":"2022-10-12 17:41:16.000000000","message":"The test looks good and it\u0027s executed by the gate jobs successfully","commit_id":"6714b65a210c5851591dfd5dfe9ad1cbf1fa49e9"}],"tempest/api/compute/images/test_images.py":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"2186772edfa034e4979bfa4ae0ed36a30d2fa787","unresolved":true,"context_lines":[{"line_number":144,"context_line":"        self.addCleanup(self.client.delete_image, image[\u0027id\u0027])"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        # Try to create another server from that snapshot"},{"line_number":147,"context_line":"        server2 \u003d self.create_test_server(wait_until\u003d\u0027ACTIVE\u0027,"},{"line_number":148,"context_line":"                                          image_id\u003dimage[\u0027id\u0027])"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"        # Delete server 2 before we finish otherwise we\u0027ll race with"},{"line_number":151,"context_line":"        # the cleanup which tries to delete the image before the"},{"line_number":152,"context_line":"        # server is gone."},{"line_number":153,"context_line":"        waiters.wait_for_server_termination(self.servers_client, server2[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":6,"id":"a08fc59e_d1e29426","line":153,"range":{"start_line":147,"start_character":0,"end_line":153,"end_character":79},"updated":"2022-10-07 20:32:20.000000000","message":"you still need to delete the server2 before wait_for_server_termination","commit_id":"eda85800eae0c97a45bc4ad3c068d167b1f07ced"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5434f83b26357f087ace3135edc8a37a19d7e837","unresolved":false,"context_lines":[{"line_number":144,"context_line":"        self.addCleanup(self.client.delete_image, image[\u0027id\u0027])"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        # Try to create another server from that snapshot"},{"line_number":147,"context_line":"        server2 \u003d self.create_test_server(wait_until\u003d\u0027ACTIVE\u0027,"},{"line_number":148,"context_line":"                                          image_id\u003dimage[\u0027id\u0027])"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"        # Delete server 2 before we finish otherwise we\u0027ll race with"},{"line_number":151,"context_line":"        # the cleanup which tries to delete the image before the"},{"line_number":152,"context_line":"        # server is gone."},{"line_number":153,"context_line":"        waiters.wait_for_server_termination(self.servers_client, server2[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":6,"id":"4b8d5b72_132a1e3f","line":153,"range":{"start_line":147,"start_character":0,"end_line":153,"end_character":79},"in_reply_to":"a08fc59e_d1e29426","updated":"2022-10-11 14:48:02.000000000","message":"Done","commit_id":"eda85800eae0c97a45bc4ad3c068d167b1f07ced"}]}