)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"1deeb1ecfaa0b17ea7695b0f6a9640b018521b15","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"d92d8d28_d470fdcb","updated":"2023-09-04 16:25:17.000000000","message":"small nit inline but LGTM.","commit_id":"a43b434125df26021025d76c3acde96e7144fff3"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"48c74611a68902ef83ba53a6b7cfe4ab1aee3a27","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"aae24772_27d3bda3","updated":"2023-09-05 17:20:03.000000000","message":"I agree to have some negative test but tempest is not the good place to have those. There can be lot of test we can add for request parameter negative. I think it is covered in cinder unit or functional tests, if not then that is the place we should add this test,\n\nhttps://docs.openstack.org/tempest/latest/HACKING.html#negative-tests","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"8aa5b6b7a0c675cc2370891aedf5266e60d12819","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"571c7230_f61c2cd3","updated":"2023-09-04 18:35:37.000000000","message":"LGTM","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":19262,"name":"Liron Kuchlani","email":"lkuchlan@redhat.com","username":"lkuchlan"},"change_message_id":"bfe09d1670db40ec1c843a9c50e6754baa1aadd1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"888c6422_aeb621f2","updated":"2023-09-05 06:10:12.000000000","message":"LGTM!","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"fc96254d09463d510045192eb52fc21c30a3ae39","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d089fb1d_90707ac3","updated":"2023-09-05 11:31:03.000000000","message":"agree, lgtm","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"f77a36a8110cf867c0be3fede1124af32f23dd21","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"42325d00_fe261e78","updated":"2023-09-12 07:28:51.000000000","message":"moved to https://review.opendev.org/c/openstack/cinder-tempest-plugin/+/894189","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"bca1e8ba0c77c0a602e4ad734f6860d33bce49dd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ef7ba0b8_86d41714","in_reply_to":"0d11f9a7_70681104","updated":"2023-09-06 03:18:49.000000000","message":"Yes, we have those existing tests but we kept them only for interop otherwise all those who touch DB or API layer in services can be tested at service side unit or functional test. After existing tests needed by interop and to make sure it does not get added more new one, we wrote that policy around negative tests. That is reason I have been rejecting such many test cases in past too\n\n- https://review.opendev.org/q/project:openstack/tempest+negative+test+owner:wangzhiguang%2540inspur.com","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"f206b7aacbbcc22ed52709378b4530857cfc407c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c15a5ecf_a0c70309","in_reply_to":"58d14dac_9ddee4b4","updated":"2023-09-06 17:34:30.000000000","message":"Ok I think we are just reading the guidlines and what is negatice tests which is nothing but for the 1. existing negative test 2. if interop need any negative test then we need to add one.\n\nThe below line explicitly say about Tempest is not right place for adding MORE negative tests unless it is required by the interop guidlines\n\n\"....Tempest is not suitable for handling all negative test cases, as the wide variety and complexity of negative tests can lead to long test runs and knowledge of internal implementation details. The bulk of negative testing should be handled with project function tests......\"\n\n- https://docs.openstack.org/tempest/latest/HACKING.html#negative-tests\n\nIf it is coming from the interop guidelines as you mentioned it can be a good interop test then yes we can add it in Tempest otherwise same test coverage if present in cinder unit/functional test so that is enough to verify the interopability of this API (instead all the request param not just this one as special).\n\nThese are the main reason I keep 1 on all the negative tests (not required by interop guidelines) and asked author to add it in project unit/functional tests.","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"d2d08e300a2cccdf0634e7b84d2ea877c9e4292c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0d11f9a7_70681104","in_reply_to":"aae24772_27d3bda3","updated":"2023-09-05 20:42:27.000000000","message":"right, I understand the side Tempest is on in regards of negative tests, however, to quote the documentation:\n\"... Tempest is not suitable for handling all negative test cases ...\"\n\nThis patch complies with that as it adds only one test to ensure that the old way does not really work anymore. If I think of any other negative tests we have in Tempest, they do the same, they test one (or just a few) very specific error handling path. One practical situation, I can think of right away, might be running Tempest after update/upgrade - we wanna fail if the cloud still accepts using the API the old way.","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"ab7a5b08f103045057d1ee0c4df871472a9a6f1f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"58d14dac_9ddee4b4","in_reply_to":"ef7ba0b8_86d41714","updated":"2023-09-06 07:47:34.000000000","message":"The guideline you mentioned (https://specs.openstack.org/openstack/api-wg/guidelines/http/response-codes.html#failure-code-clarifications\n) are about handling error situations, which I believe is the case here, as the change that led to this touched several components and it\u0027s a bit risky to ignore it. \nWhile this is not an \"unexpected parameter\", I would say this is a (rare) variation of \"If a request contains an unexpected attribute in the body, the server should return a 400 Bad Request response. \"\n\nIt is also fine according the additional rule \"About BadRequest(HTTP400) case: We can add a single negative tests of BadRequest for each resource and method(POST, PUT). \"\n\nSo I would say this is a case that it is worth adding for interoperability purposes.","commit_id":"5882c37e33f063c31909abba064806d34b1ec5fa"}],"tempest/api/compute/volumes/test_attach_volume_negative.py":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"1deeb1ecfaa0b17ea7695b0f6a9640b018521b15","unresolved":true,"context_lines":[{"line_number":86,"context_line":"    @decorators.attr(type\u003d[\u0027negative\u0027])"},{"line_number":87,"context_line":"    @decorators.idempotent_id(\u002773ebb595-efb6-4378-bd73-f0ca83c60328\u0027)"},{"line_number":88,"context_line":"    def test_multiattach_removed_api(self):"},{"line_number":89,"context_line":"        \"\"\"Test which creates a multiattach volume using the old API"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        1. Try to create a volume with multiattach: true"},{"line_number":92,"context_line":"        2. Expect a BadRequest exception, with a specific error message"}],"source_content_type":"text/x-python","patch_set":1,"id":"63804669_a50262e1","line":89,"range":{"start_line":89,"start_character":61,"end_line":89,"end_character":68},"updated":"2023-09-04 16:25:17.000000000","message":"nit: it\u0027s not exactly old API but old way of passing the multiattach parameter that is not accepted now","commit_id":"a43b434125df26021025d76c3acde96e7144fff3"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"7e02eee610e365f4a0e270aea026212316eb74b5","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    @decorators.attr(type\u003d[\u0027negative\u0027])"},{"line_number":87,"context_line":"    @decorators.idempotent_id(\u002773ebb595-efb6-4378-bd73-f0ca83c60328\u0027)"},{"line_number":88,"context_line":"    def test_multiattach_removed_api(self):"},{"line_number":89,"context_line":"        \"\"\"Test which creates a multiattach volume using the old API"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        1. Try to create a volume with multiattach: true"},{"line_number":92,"context_line":"        2. Expect a BadRequest exception, with a specific error message"}],"source_content_type":"text/x-python","patch_set":1,"id":"04620e78_6e3d3036","line":89,"range":{"start_line":89,"start_character":61,"end_line":89,"end_character":68},"in_reply_to":"63804669_a50262e1","updated":"2023-09-04 18:00:37.000000000","message":"Fixed","commit_id":"a43b434125df26021025d76c3acde96e7144fff3"}]}