Repository: google/gnxi (language: python)

Issue:
oc_config_validate retries gNMI Set messages, it should only retry Gets
oc_config_validate can retry gNMI messages when assertions fail. This is to account for different delays in reflecting back configuration or state.

However, currently it is also retrying gNMI Set messages. This is not correct, as a Target needs only 1 successful SET message to do its job.

The retry mechanism must only apply to gNMI Get messages.


Discussion / hints:

Fix patch:
diff --git a/oc_config_validate/oc_config_validate/__init__.py b/oc_config_validate/oc_config_validate/__init__.py
index b19ee4b7..ba51cedf 100644
--- a/oc_config_validate/oc_config_validate/__init__.py
+++ b/oc_config_validate/oc_config_validate/__init__.py
@@ -1,1 +1,1 @@
-__version__ = "2.2.1"
+__version__ = "2.2.2"
diff --git a/oc_config_validate/oc_config_validate/target.py b/oc_config_validate/oc_config_validate/target.py
index e6b87c93..920a2e14 100644
--- a/oc_config_validate/oc_config_validate/target.py
+++ b/oc_config_validate/oc_config_validate/target.py
@@ -28,8 +28,7 @@
 from typing import Any, Dict, Iterator, List, Optional, Tuple
 
 import grpc
-from grpc._channel import _InactiveRpcError
-from grpc._channel import _MultiThreadedRendezvous
+from grpc._channel import _InactiveRpcError, _MultiThreadedRendezvous
 
 from oc_config_validate import context, schema
 from oc_config_validate.gnmi import gnmi_pb2, gnmi_pb2_grpc  # type: ignore


Test patch:
diff --git a/oc_config_validate/oc_config_validate/testbase.py b/oc_config_validate/oc_config_validate/testbase.py
index ddacacb6..d6a3b789 100644
--- a/oc_config_validate/oc_config_validate/testbase.py
+++ b/oc_config_validate/oc_config_validate/testbase.py
@@ -371,7 +371,7 @@ def log(self, formatter, *args):
         """
         self.log_message += formatter % args + '\n'
 
-    def startTest(self, test: TestCase):
+    def startTest(self, test: TestCase):  # pytype: disable=signature-mismatch
         """Clean the log before every test."""
         super().startTest(test)
         self.log_message = ''
@@ -394,12 +394,12 @@ def addFailure(self, test: TestCase, err):
         self._mirrorOutput = True
         logging.error("%s - FAIL:\n%s", test, log)
 
-    def addSuccess(self, test: TestCase):
+    def addSuccess(self, test: TestCase):   # pytype: disable=signature-mismatch  # noqa
         """Store the log messages for every successful test."""
         self.successes.append((test, self.log_message))
         logging.debug("%s - PASS:\n%s", test, self.log_message)
 
-    def addSkip(self, test: TestCase, reason: str):
+    def addSkip(self, test: TestCase, reason: str):   # pytype: disable=signature-mismatch  # noqa
         """Override parent addFailure to support logging."""
         self.skipped.append((test, reason))
         logging.debug("%s - SKIP:\n%s", test, reason)
diff --git a/oc_config_validate/oc_config_validate/testcases/config_state.py b/oc_config_validate/oc_config_validate/testcases/config_state.py
index 95580993..64df7ef0 100644
--- a/oc_config_validate/oc_config_validate/testcases/config_state.py
+++ b/oc_config_validate/oc_config_validate/testcases/config_state.py
@@ -26,8 +26,7 @@ class SetConfigCheckState(testbase.TestCase):
     json_value = None
     model = ""
 
-    @testbase.retryAssertionError
-    def testSetConfigCheckState(self):
+    def setUp(self):
         self.assertArgs(["xpath", "model", "json_value"])
         xpath = self.xpath.rstrip("/") + "/config"
         self.assertXpath(xpath)
@@ -38,11 +37,8 @@ def testSetConfigCheckState(self):
         self.assertJsonModel(json.dumps(self.json_value), model,
                              "JSON value to Set does not match the model")
 
-        # gNMI Set on /config
-        self.assertTrue(
-            self.gNMISetUpdate(xpath, self.json_value),
-            "gNMI Set did not succeed.")
-
+    @testbase.retryAssertionError
+    def _get_check_retry(self):
         # gNMI Get on /config
         xpath = self.xpath.rstrip("/") + "/config"
         resp_val = self.gNMIGetJson(xpath)
@@ -67,6 +63,14 @@ def testSetConfigCheckState(self):
         cmp, diff = schema.intersectCmp(self.json_value, got)
         self.assertTrue(cmp, diff)
 
+    def testSetConfigCheckState(self):
+        # gNMI Set on /config
+        xpath = self.xpath.rstrip("/") + "/config"
+        self.assertTrue(
+            self.gNMISetUpdate(xpath, self.json_value),
+            "gNMI Set did not succeed.")
+        self._get_check_retry()
+
 
 class DeleteConfigCheckState(testbase.TestCase):
     """Deletes the xpath and checks the /config and /state container after.
@@ -83,11 +87,24 @@ class DeleteConfigCheckState(testbase.TestCase):
     """
     xpath = ""
 
-    @testbase.retryAssertionError
-    def testDeleteConfigCheckState(self):
+    def setUp(self):
         self.assertArgs(["xpath"])
         self.assertXpath(self.xpath)
 
+    @testbase.retryAssertionError
+    def _get_check_retry(self):
+        # gNMI Get on /config to verify it is deleted
+        self.assertFalse(
+            self.gNMIGet(self.xpath.rstrip("/") + "/config"),
+            "There is still a /config container for the xpath.")
+
+        # gNMI Get on /state to verify it is deleted
+        self.assertFalse(
+            self.gNMIGet(self.xpath.rstrip("/") + "/state"),
+            "There is still a /state container for the xpath.")
+
+    def testDeleteConfigCheckState(self):
+
         # gNMI Get on /config to verify it is configured
         self.assertTrue(
             self.gNMIGet(self.xpath.rstrip("/") + "/config"),
@@ -98,12 +115,5 @@ def testDeleteConfigCheckState(self):
             self.gNMISetDelete(self.xpath.rstrip("/")),
             "gNMI Delete did not succeed.")
 
-        # gNMI Get on /config to verify it is deleted
-        self.assertFalse(
-            self.gNMIGet(self.xpath.rstrip("/") + "/config"),
-            "There is still a /config container for the xpath.")
-
-        # gNMI Get on /state to verify it is deleted
-        self.assertFalse(
-            self.gNMIGet(self.xpath.rstrip("/") + "/state"),
-            "There is still a /state container for the xpath.")
+        # gNMI Gets to confirm deletion
+        self._get_check_retry()
diff --git a/oc_config_validate/oc_config_validate/testcases/setget.py b/oc_config_validate/oc_config_validate/testcases/setget.py
index 28b2b0f9..6907f498 100644
--- a/oc_config_validate/oc_config_validate/testcases/setget.py
+++ b/oc_config_validate/oc_config_validate/testcases/setget.py
@@ -17,8 +17,8 @@ class TestCase(testbase.TestCase):
     json_value = None
     model = None
 
-    def _validate_args(self):
-        """Check testcase arguments."""
+    def setUp(self):
+        # Check testcase arguments
         self.assertArgs(["xpath", "json_value", "model"])
         self.assertXpath(self.xpath)
         self.assertIsInstance(self.json_value, dict,
@@ -27,6 +27,9 @@ def _validate_args(self):
         model = schema.ocContainerFromPath(self.model, self.xpath)
         self.assertJsonModel(json.dumps(self.json_value), model,
                              "JSON value to Set does not match the model")
+        # Set Update
+        self.assertTrue(self.gNMISetUpdate(self.xpath, self.json_value),
+                        "gNMI Set did not succeed.")
 
 
 class SetGetJsonCheck(TestCase):
@@ -48,9 +51,6 @@ class SetGetJsonCheck(TestCase):
     @testbase.retryAssertionError
     def testSetGetJsonCheck(self):
         """"""
-        self._validate_args()
-        self.assertTrue(self.gNMISetUpdate(self.xpath, self.json_value),
-                        "gNMI Set did not succeed.")
         resp = self.gNMIGet(self.xpath)
         self.assertIsNotNone(resp, "No gNMI GET response")
         self.resp_val = resp.json_ietf_val
@@ -80,10 +80,6 @@ class SetGetJsonCheckCompare(TestCase):
 
     @testbase.retryAssertionError
     def testSetGetJsonCheckCompare(self):
-        """"""
-        self._validate_args()
-        self.assertTrue(self.gNMISetUpdate(self.xpath, self.json_value),
-                        "gNMI Set did not succeed.")
         resp = self.gNMIGet(self.xpath)
         self.assertIsNotNone(resp, "No gNMI GET response")
         self.resp_val = resp.json_ietf_val