[GitHub] [trafficserver] bneradt commented on a change in pull request #7866: Autest - Proxy Verifier Extension, add context template $-base string…

[GitBox <gi...@apache.org>]

bneradt commented on a change in pull request #7866:
URL: https://github.com/apache/trafficserver/pull/7866#discussion_r636930438



##########
File path: tests/gold_tests/autest-site/verifier_common.py
##########
@@ -33,3 +36,23 @@ def create_address_argument(ports):
         argument += "127.0.0.1:{}".format(port)
     argument += '"'
     return argument
+
+# Perform substitution base on the passed context dict.
+# This function will return the new replay_path file
+
+

Review comment:
       Comment blocks for functions in Python generally go inside the function. See `create_address_arguments` above. Thus:
   
   ```
   def substitute_context_in_replay_file(process, replay_path, context):
       """
       Perform substitution based upon the passed context dict.
       This function will return the new replay_path file
       """
   ```

##########
File path: tests/gold_tests/autest-site/verifier_common.py
##########
@@ -33,3 +36,23 @@ def create_address_argument(ports):
         argument += "127.0.0.1:{}".format(port)
     argument += '"'
     return argument
+
+# Perform substitution base on the passed context dict.
+# This function will return the new replay_path file
+
+
+def substitute_context_in_replay_file(process, replay_path, context):
+    # Only files for now
+    if os.path.isdir(replay_path):
+        raise ValueError(f"Mapping substitution not supported for directories.")
+
+    with open(os.path.join(process.TestDirectory, replay_path), 'r') as replay_file:
+        src = Template(replay_file.read())
+        result = src.substitute(context)

Review comment:
       I suggest:
   * src -> replay_template
   * result -> replay_text

##########
File path: tests/gold_tests/autest-site/verifier_client.test.ext
##########
@@ -163,15 +169,27 @@ def AddVerifierClientProcess(run, name, replay_path, http_ports=None,
 
         other_args: (str) Any other arbitrary options to pass to verifier-client.
 
+        context: Any dictionary-like object with keys that match the placeholders
+                 in the replay file.
+                 Template strings support $-based substitutions is supported in the
+                 replay file.
+                 You can refer to https://docs.python.org/3/library/string.html#template-strings
+                 for more information how to add template strings to the replay file.
     Returns:
         The newly constructed verifier-client for the test run, which is also the
         Default Process of the test run.
+    Throws:
+        ValueError:
+            If context substitution is expected to be done but a directory is passed as a
+            replay_path.
+        OSError in case of any issues related to I/O error, ie: File Not found for the replay
+            file when a context substitution is expected.

Review comment:
       Will a `KeyError` be raised if `context` is missing variables in the replay template?

##########
File path: tests/gold_tests/autest-site/verifier_server.test.ext
##########
@@ -174,19 +180,29 @@ def MakeVerifierServerProcess(test, name, replay_path, http_ports=None,
 
         other_args: (str) Any other arbitrary options to pass to verifier-server.
 
+        context: Any dictionary-like object with keys that match the placeholders
+                 in the replay file.
+                 Template strings support $-based substitutions is supported in the
+                 replay file.

Review comment:
       Looks like we want to remove: "is supported"

##########
File path: tests/gold_tests/autest-site/verifier_common.py
##########
@@ -33,3 +36,23 @@ def create_address_argument(ports):
         argument += "127.0.0.1:{}".format(port)
     argument += '"'
     return argument
+
+# Perform substitution base on the passed context dict.
+# This function will return the new replay_path file
+
+
+def substitute_context_in_replay_file(process, replay_path, context):
+    # Only files for now
+    if os.path.isdir(replay_path):
+        raise ValueError(f"Mapping substitution not supported for directories.")
+
+    with open(os.path.join(process.TestDirectory, replay_path), 'r') as replay_file:
+        src = Template(replay_file.read())
+        result = src.substitute(context)
+        tf = tempfile.NamedTemporaryFile(delete=False, dir=process.RunDirectory, suffix=f"_{os.path.basename(replay_path)}")
+        with open(tf.name, "w") as new_replay_file:
+            new_replay_file.write(result)
+
+        replay_path = tf.name

Review comment:
       If you move this before line 53, you can use it in the open.

##########
File path: tests/gold_tests/autest-site/verifier_client.test.ext
##########
@@ -163,15 +169,27 @@ def AddVerifierClientProcess(run, name, replay_path, http_ports=None,
 
         other_args: (str) Any other arbitrary options to pass to verifier-client.
 
+        context: Any dictionary-like object with keys that match the placeholders
+                 in the replay file.
+                 Template strings support $-based substitutions is supported in the
+                 replay file.

Review comment:
       Looks like we want to remove: "is supported"




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org