Refactor Python testing scripts and utilities - Part 1 #3855

fruffy · 2023-01-23T15:53:09Z

Replace the custom defined "outputs" with an actual logger that prints and logs to a file as needed
Use modern Python3 code (f-strings, pathlib, etc)
Use subprocess.run instead of opening a process than running it.

fruffy · 2023-01-24T19:51:50Z

@vlstill This is a big PR, because it refreshes a lot of the old Python code. Still, among others, it refactors the test utilities to use subprocess.run instead of check_output etc.

After this PR, I am planning to generalize ebpfenv, such that it can be used as a general virtual namespace environment. Finally, add typing information to all the Python utilities. You had some comments on the PTF PR, are those resolved or do you have any other suggestions?

fruffy · 2023-01-24T19:58:53Z

@mbudiu-vmw This PR makes a lot of changes in the testutils library and migrates to actual logging for the test scripts from the custom logging that we had. It does not touch the Python scripts which do not depend on testutils, such as run-bmv2-test.py etc.

@tatry @osinstom This PR touches ubpf scripts, too. Any dependencies I should keep in mind there?

mihaibudiu · 2023-01-24T21:21:07Z

backends/bmv2/run-bmv2-ptf-test.py

 import random
+import logging


I will let the owners of this file review the changes

That's us! We approve :)

mihaibudiu · 2023-01-24T21:23:10Z

backends/bmv2/run-bmv2-test.py

    try:
        result = process_file(options, argv)
    except Exception as e:
-        print("Exception ", e)
-        sys.exit(FAILURE)
+        print(f"There was a problem parsing STF file {options.testFile}:")


why is this always a parsing problem?

mihaibudiu · 2023-01-24T21:25:19Z

backends/ebpf/run-ebpf-test.py

-        # Actual location of the test framework
-        self.testdir = os.path.dirname(os.path.realpath(__file__))
-        self.extern = ""                # Path to C file with extern definition
+        self.binary = ""  # This program's name.


I find the original version easier to read. Why spend time reformatting whitespace?

mihaibudiu · 2023-01-24T21:26:25Z

backends/ebpf/targets/bcc_target.py

 import sys
+from pathlib import Path


is this file even used?

mihaibudiu · 2023-01-24T21:26:56Z

backends/ebpf/targets/ebpfenv.py

 import sys
-from subprocess import Popen, PIPE
+import logging


I will let the owners review this file

mihaibudiu · 2023-01-24T21:31:07Z

backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake

@@ -40,7 +40,8 @@ p4tools_add_xfail_reason(

 p4tools_add_xfail_reason(
  "testgen-p4c-bmv2-ptf"
-  "Invalid reference to object of type"
+  "Segmentation fault"


This is not a good sign.
I see no reason for the changes in the test scripts to lead to a segfault, something must be wrong somewhere.

In both cases the segmentation fault is thrown.

The underlying cause is a bit tricky. Previously, simple_switch_grpc was invoked with --log-console, which directly printed output to stdout/err. However, this occasionally overwhelmed the process pipe and the process got stuck.

Now, we use --log-file, but simple switch crashes before the statement can be logged/generated. So it does not appear anymore.

The completely solution is to either fix simple_switch's logging or to fix the Popen call so that it does not get stuck.

If it's the same bug please file an issue and add a comment to the makefile.

I believe it is this bug https://github.com/p4lang/behavioral-model/issues/1124

I am not aware of a corresponding bug for p4c, we should have one. Can you please add one?

I asked Antonin to migrate the issue.

vlstill

I was trying primarily to look at the Python side of the change, I did not check overall design. I think this goes in the right direction and I have a few suggestions in the code.

I'd suggest running the code through some python formatting checker such as pycodestyle and possibly through flake8 (linter). I think that for languages that have specified code style, it generally makes sense to adhere to it (even though I might personally disagree with some python style decisions).

vlstill · 2023-01-25T07:41:33Z

backends/bmv2/base_test.py

+FILE_DIR = Path(__file__).resolve().parent
+TOOLS_PATH = FILE_DIR.joinpath("../../tools")
+sys.path.append(str(TOOLS_PATH))
+import testutils


Maybe add a comment that this import needs the path modification?

Really dislike this boilerplate code actually. Do you have a better idea how to handle relative parent-level imports? Without requiring the user to input the path every time.

vlstill · 2023-01-25T07:44:11Z

backends/bmv2/base_test.py

-        return partial(self.func, instance, *(self.args or ()),
-                       **(self.keywords or {}))
+        return partial(self.func, instance, *(self.args or ()), **(self.keywords or {}))


This (and similar changes) now goes over the 79 character limit of Python's formating conventions. I don't think that is a good idea, many (at least semi-intelligent) code editors will highlight that as a problem. If we want to go against some of the python style guides, I think we should have an override file that informs pycodestyle and similar programs what is to be ignored.

I have been using a .yapf.style and a .pylintrc file with a character limit set to 100. I used 100 because our C code also uses 100 as character limit. I can add this file and its formatting in a separate PR?

I have been not using black since it is a little too opinionated. There are so many options for style tools, I am actually not sure which we should use.

I see. Is there any reason to not add the style config files in this PR?

This PR is already big, so I was hoping to do the formatting in a separate PR. Then we can also agree on a style there.

vlstill · 2023-01-25T07:48:33Z

backends/bmv2/base_test.py

@@ -246,22 +248,19 @@ def __exit__(self, exc_type, exc_value, tb):
        if not issubclass(exc_type, P4RuntimeWriteException):
            # let unexpected exceptions pass through
            return False
-        self.exception = exc_value  # store for later retrieval
+        self.exception = exc_value # store for later retrieval


Again, this was formatted according to the python style guide (at least two spaces before comment) and now goes directly against it.

Having formatting that complies with Mihai's request and the two space rule is unfortunately quite difficult. I have not been able to find a configuration that does both (Aligning comments as well as maintaining a minimum distance).

Personally, I prefer to have no inline comments at all since they often run over the maximum character length.

google/yapf#675

This is the issue I am facing. But other formatters are also not better at this. It might be less headache to forbid inline comments in general.

vlstill · 2023-01-25T07:51:36Z

backends/bmv2/bmv2stf.py

 def ByteToHex(byteStr):
-    return ''.join( [ ("%02X" % x) for x in byteStr ] )
+    return ''.join([("%02X" % x) for x in byteStr])


Suggested change

return ''.join([("%02X" % x) for x in byteStr])

return ''.join(("%02X" % x) for x in byteStr)

(join can accept an iterator expression directly)

vlstill · 2023-01-25T07:52:51Z

backends/bmv2/bmv2stf.py

@@ -249,13 +282,14 @@ def makeMask(self, value):
            value = value[2:]
            prefix = "0o"
        else:
-            raise Exception("Decimal value "+value+" not supported for ternary key")
+            raise Exception("Decimal value " + value + " not supported for ternary key")


or

Suggested change

raise Exception("Decimal value " + value + " not supported for ternary key")

raise Exception(f"Decimal value {value} not supported for ternary key")

I have not touched bmv2stf.py yet except for a little formatting. The code is very old and written for Python 2.7. I would like to do this in a separate PR.

backends/ebpf/targets/target.py

vlstill · 2023-01-25T08:18:02Z

backends/ebpf/targets/target.py

            # If the compiler crashed fail the test
-            if 'Compiler Bug' in open(self.outputs["stderr"]).readlines():
-                sys.exit(FAILURE)
+            if "Compiler Bug" in str(out):


is out binary?

tools/testutils.py

vlstill · 2023-01-25T08:32:52Z

tools/testutils.py

    return proc


-def run_process(verbose, proc, timeout, outputs, errmsg):
-
+def run_process(proc, timeout, errmsg):


The name is a bit misleading, it suggest the process is executed, but in fact only its outputs are consumed.

tools/testutils.py

osinstom · 2023-01-25T13:36:52Z

@fruffy it's been a while since we worked on the uBPF backend, but there should not be any specific dependencies to be aware of.

fruffy · 2023-01-30T23:43:37Z

@mbudiu-vmw Any concerns on your side that have not been resolved yet?

mihaibudiu · 2023-01-31T00:40:21Z

This is not my favorite part of the compiler, so I try to avoid it. If you promise to take care of it, it's yours.

fruffy · 2023-01-31T00:49:49Z

This is not my favorite part of the compiler, so I try to avoid it. If you promise to take care of it, it's yours.

Thanks! A lot of the code that was touched in this PR I have written at some point, so I am on the hook anyway.

fruffy force-pushed the fruffy/refactor_testing_scripts branch 4 times, most recently from 685147d to 492d22b Compare January 24, 2023 16:38

fruffy marked this pull request as ready for review January 24, 2023 19:47

fruffy requested review from pkotikal, vhavel, jnfoster and mihaibudiu as code owners January 24, 2023 19:47

mihaibudiu reviewed Jan 24, 2023

View reviewed changes

vlstill reviewed Jan 25, 2023

View reviewed changes

fruffy force-pushed the fruffy/refactor_testing_scripts branch 2 times, most recently from 770b0b2 to 20143f1 Compare January 25, 2023 14:46

fruffy added 6 commits January 26, 2023 10:43

Refactor the testing Python scripts and utilities.

36e757a

Xfail edits.

af67340

Apply consistent formatting using yapf.

0a4d279

Review comments.

4531eac

Review comments.

65170d5

More f-strings.

ece0074

fruffy force-pushed the fruffy/refactor_testing_scripts branch from 20143f1 to ece0074 Compare January 26, 2023 15:44

mihaibudiu approved these changes Jan 31, 2023

View reviewed changes

fruffy merged commit 64285c4 into main Jan 31, 2023

fruffy deleted the fruffy/refactor_testing_scripts branch January 31, 2023 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Python testing scripts and utilities - Part 1 #3855

Refactor Python testing scripts and utilities - Part 1 #3855

fruffy commented Jan 23, 2023

fruffy commented Jan 24, 2023 •

edited

Loading

fruffy commented Jan 24, 2023

mihaibudiu Jan 24, 2023

fruffy Jan 24, 2023 •

edited

Loading

mihaibudiu Jan 24, 2023

mihaibudiu Jan 24, 2023

mihaibudiu Jan 24, 2023

mihaibudiu Jan 24, 2023

mihaibudiu Jan 24, 2023

fruffy Jan 24, 2023

mihaibudiu Jan 24, 2023

fruffy Jan 24, 2023

mihaibudiu Jan 24, 2023

fruffy Jan 26, 2023

vlstill left a comment

vlstill Jan 25, 2023

fruffy Jan 26, 2023

vlstill Jan 25, 2023

fruffy Jan 25, 2023

vlstill Jan 25, 2023

fruffy Jan 25, 2023

vlstill Jan 25, 2023

fruffy Jan 25, 2023

fruffy Jan 25, 2023

vlstill Jan 25, 2023

vlstill Jan 25, 2023

fruffy Jan 25, 2023

vlstill Jan 25, 2023

vlstill Jan 25, 2023

osinstom commented Jan 25, 2023

fruffy commented Jan 30, 2023

mihaibudiu commented Jan 31, 2023

fruffy commented Jan 31, 2023

	return ''.join([("%02X" % x) for x in byteStr])
	return ''.join(("%02X" % x) for x in byteStr)

	raise Exception("Decimal value " + value + " not supported for ternary key")
	raise Exception(f"Decimal value {value} not supported for ternary key")

Refactor Python testing scripts and utilities - Part 1 #3855

Refactor Python testing scripts and utilities - Part 1 #3855

Conversation

fruffy commented Jan 23, 2023

fruffy commented Jan 24, 2023 • edited Loading

fruffy commented Jan 24, 2023

Choose a reason for hiding this comment

fruffy Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osinstom commented Jan 25, 2023

fruffy commented Jan 30, 2023

mihaibudiu commented Jan 31, 2023

fruffy commented Jan 31, 2023

fruffy commented Jan 24, 2023 •

edited

Loading

fruffy Jan 24, 2023 •

edited

Loading