From e92a0dc5024e4d6f7bcbbe9fde8ee50d6be0d1ed Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 21 Jan 2025 10:54:33 -0700 Subject: [PATCH 1/5] Rework python script_instrumentor.py and test --- .github/workflows/api_tests.yml | 35 +++- .../scripts/running_script.py | 4 + .../scripts/script_instrumentor.py | 168 +++++++++++----- .../test/__init__.py | 0 .../test/scripts/test_script_instrumentor.py | 185 ++++++++++++++++++ 5 files changed, 346 insertions(+), 46 deletions(-) create mode 100644 openc3-cosmos-script-runner-api/test/__init__.py create mode 100644 openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py diff --git a/.github/workflows/api_tests.yml b/.github/workflows/api_tests.yml index f4857c8d4a..310b113c2a 100644 --- a/.github/workflows/api_tests.yml +++ b/.github/workflows/api_tests.yml @@ -60,7 +60,7 @@ jobs: flags: ruby-api # See codecov.yml token: ${{ secrets.CODECOV_TOKEN }} - script-runner-api: + script-runner-api-ruby: if: ${{ github.actor != 'dependabot[bot]' }} runs-on: ubuntu-latest strategy: @@ -90,3 +90,36 @@ jobs: directory: openc3-cosmos-script-runner-api/coverage flags: ruby-api # See codecov.yml token: ${{ secrets.CODECOV_TOKEN }} + + script-runner-api-python: + if: ${{ github.actor != 'dependabot[bot]' }} + runs-on: ubuntu-latest + strategy: + matrix: + python-version: ["3.10", "3.11"] + + steps: + - uses: actions/checkout@v4 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + working-directory: openc3-cosmos-script-runner-api + - name: Lint with ruff + run: | + ruff --format=github scripts/*.py + working-directory: openc3-cosmos-script-runner-api + - name: Run unit tests + run: | + coverage run -m pytest ./test/ + coverage xml -i + working-directory: openc3-cosmos-script-runner-api + - uses: codecov/codecov-action@v5 + with: + working-directory: openc3/python + flags: python # See codecov.yml + token: ${{ secrets.CODECOV_TOKEN }} diff --git a/openc3-cosmos-script-runner-api/scripts/running_script.py b/openc3-cosmos-script-runner-api/scripts/running_script.py index 8eb3070f2b..603a5ed49c 100644 --- a/openc3-cosmos-script-runner-api/scripts/running_script.py +++ b/openc3-cosmos-script-runner-api/scripts/running_script.py @@ -489,6 +489,7 @@ def instrument_script(cls, text, filename): parsed = ast.parse(text) tree = ScriptInstrumentor(filename).visit(parsed) + # Normal Python code is run with mode='exec' whose root is ast.Module result = compile(tree, filename=filename, mode="exec") return result @@ -1158,10 +1159,13 @@ def handle_exception( self.mark_error() self.wait_for_go_or_stop_or_retry(exc_value) + # See script_instrumentor.py for how retry is used if self.retry_needed: self.retry_needed = False + # Return True to the instrumented code to retry the line return True else: + # Return False to the instrumented code to break the while loop return False def load_file_into_script(self, filename): diff --git a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py index f2759c3ba3..5f71e177d8 100644 --- a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py @@ -17,6 +17,15 @@ import ast +# For details on the AST, see https://docs.python.org/3/library/ast.html +# and https://greentreesnakes.readthedocs.io/en/latest/nodes.html + +# This class is used to instrument a Python script with calls to a +# RunningScript instance. The RunningScript instance is used to +# track the execution of the script, and can be used to pause and +# resume the script. We inherit from ast.NodeTransformer, which +# allows us to modify the AST of the script. We override the visit +# method for each type of node that we want to instrument. class ScriptInstrumentor(ast.NodeTransformer): pre_line_instrumentation = """ RunningScript.instance.pre_line_instrumentation('{}', {}, globals(), locals()) @@ -38,93 +47,162 @@ def __init__(self, filename): self.filename = filename self.in_try = False - # These are statements which should have an enter and leave - # (In retrospect, this isn't always true, eg, for 'if') - def track_enter_leave_lineno(self, node): + # What we're trying to do is wrap executable statements in a while True try/except block + # For example if the input code is "print('HI')", we want to transform it to: + # while True: + # try: + # RunningScript.instance.pre_line_instrumentation('myfile.py', 1, globals(), locals()) + # --> print('HI') <-- This is the original node + # break + # except: + # retry_needed = RunningScript.instance.exception_instrumentation('myfile.py', 1) + # if retry_needed: + # continue + # else: + # break + # finally: + # RunningScript.instance.post_line_instrumentation('myfile.py', 1) + # This allows us to retry statements that raise exceptions + def track_enter_leave(self, node): + # Determine if we're in a try block in_try = self.in_try if not in_try and type(node) in (ast.Try, ast.TryStar): self.in_try = True + # Visit the children of the node node = self.generic_visit(node) if not in_try and type(node) in (ast.Try, ast.TryStar): self.in_try = False - enter = ast.parse( + # ast.parse returns a module, so we need to extract + # the first element of the body which is the node + pre_line = ast.parse( self.pre_line_instrumentation.format(self.filename, node.lineno) ).body[0] - leave = ast.parse( + post_line = ast.parse( self.post_line_instrumentation.format(self.filename, node.lineno) ).body[0] true_node = ast.Constant(True) break_node = ast.Break() - for new_node in (enter, leave, true_node, break_node): + for new_node in (pre_line, post_line, true_node, break_node): + # Copy source location from the original node to our new nodes ast.copy_location(new_node, node) - # This is the code for "if 1: ..." - inhandler = ast.parse( + # Create the exception handler code node. This results in multiple nodes + # because we have a top level assignment and if statement + exception_handler = ast.parse( self.exception_instrumentation.format(self.filename, node.lineno) ).body - for new_node in inhandler: + for new_node in exception_handler: ast.copy_location(new_node, node) + # Recursively yield the children of the new_node and copy in source locations + # It's actually surprising how many nodes are nested in the new_node for new_node2 in ast.walk(new_node): ast.copy_location(new_node2, node) - excepthandler = ast.ExceptHandler(expr=None, name=None, body=inhandler) + # Create an exception handler node to wrap the exception handler code + excepthandler = ast.ExceptHandler(type=None, name=None, body=exception_handler) ast.copy_location(excepthandler, node) + # If we're not already in a try block, we need to wrap the node in a while loop if not self.in_try: try_node = ast.Try( - body=[enter, node, break_node], + # pre_line is the pre_line_instrumentation, node is the original node + # and if the code is executed without an exception, we break + body=[pre_line, node, break_node], + # Pass in the handler we created above handlers=[excepthandler], + # No else block orelse=[], - finalbody=[leave], + # The try / except finally block is the post_line_instrumentation + finalbody=[post_line], ) ast.copy_location(try_node, node) while_node = ast.While(test=true_node, body=[try_node], orelse=[]) ast.copy_location(while_node, node) return while_node + # We're already in a try block, so we just need to wrap the node in a try block else: try_node = ast.Try( - body=[enter, node], + body=[pre_line, node], handlers=[], orelse=[], - finalbody=[leave], + finalbody=[post_line], ) ast.copy_location(try_node, node) return try_node - visit_Assign = track_enter_leave_lineno - visit_AugAssign = track_enter_leave_lineno - visit_Delete = track_enter_leave_lineno - visit_Print = track_enter_leave_lineno - visit_Assert = track_enter_leave_lineno - visit_Import = track_enter_leave_lineno - visit_ImportFrom = track_enter_leave_lineno - visit_Exec = track_enter_leave_lineno - # Global - visit_Expr = track_enter_leave_lineno - - # These statements can be reached, but they change - # control flow and are never exited. - def track_reached_lineno(self, node): + # Call the pre_line_instrumentation ONLY and then exceute the node + def track_reached(self, node): + # Determine if we're in a try block, this is used by track_enter_leave + in_try = self.in_try + if not in_try and type(node) in (ast.Try, ast.TryStar): + self.in_try = True + + # Visit the children of the node node = self.generic_visit(node) - reach = ast.parse( + pre_line = ast.parse( self.pre_line_instrumentation.format(self.filename, node.lineno) ).body[0] - ast.copy_location(reach, node) + ast.copy_location(pre_line, node) - n = ast.Num(n=1) + # Create a simple constant node with the value 1 that we can use with our If node + n = ast.Constant(value=1) ast.copy_location(n, node) - if_node = ast.If(test=n, body=[reach, node], orelse=[]) + # The if_node is effectively a noop that holds the preline & node that we need to execute + if_node = ast.If(test=n, body=[pre_line, node], orelse=[]) ast.copy_location(if_node, node) return if_node - visit_With = track_reached_lineno - visit_FunctionDef = track_reached_lineno - visit_ClassDef = track_reached_lineno - visit_For = track_reached_lineno - visit_While = track_reached_lineno - visit_If = track_reached_lineno - visit_Try = track_reached_lineno - visit_TryStar = track_reached_lineno - visit_Pass = track_reached_lineno - visit_Return = track_reached_lineno - visit_Raise = track_enter_leave_lineno - visit_Break = track_reached_lineno - visit_Continue = track_reached_lineno + # Notes organized (including newlines) per https://docs.python.org/3/library/ast.html#abstract-grammar + # Nodes that change control flow are processed by track_reached, otherwise we track_enter_leave + visit_FunctionDef = track_reached + visit_AsyncFunctionDef = track_reached + + visit_ClassDef = track_reached + visit_Return = track_reached + + visit_Delete = track_enter_leave + visit_Assign = track_enter_leave + visit_TypeAlias = track_enter_leave + visit_AugAssign = track_enter_leave + visit_AnnAssign = track_enter_leave + + visit_For = track_reached + visit_AsyncFor = track_reached + visit_While = track_reached + visit_If = track_reached + visit_With = track_reached + visit_AsyncWith = track_reached + + # We can track the match statement but not any of the case statements + # because they must come unaltered after the match statement + visit_Match = track_reached + + visit_Raise = track_enter_leave + visit_Try = track_reached + visit_TryStar = track_reached + visit_Assert = track_enter_leave + + visit_Import = track_enter_leave + visit_ImportFrom = track_enter_leave + + visit_Global = track_enter_leave + visit_Nonlocal = track_enter_leave + visit_Expr = track_enter_leave + visit_Pass = track_reached + visit_Break = track_reached + visit_Continue = track_reached + + # expr nodes: mostly subnodes in assignments or return statements + # TODO: Should we handle the following: + # visit_NamedExpr = track_enter_leave + # visit_Lambda = track_enter_leave + # visit_IfExp = track_enter_leave + # visit_Await = track_reached + # visit_Yield = track_reached + # visit_YieldFrom = track_reached + # visit_Call = track_reached + # visit_JoinedStr = track_enter_leave + # visit_Constant = track_enter_leave + + # All the expr_context, boolop, operator, unaryop, cmpop nodes are not modified + # ExceptHandler must follow try or tryStar so don't modify it + # Can't modify any of pattern nodes (case) because they have to come unaltered after match + # Ignore the type_ignore and type_param nodes diff --git a/openc3-cosmos-script-runner-api/test/__init__.py b/openc3-cosmos-script-runner-api/test/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py new file mode 100644 index 0000000000..13e9d64715 --- /dev/null +++ b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py @@ -0,0 +1,185 @@ +import ast +import pytest +from scripts.script_instrumentor import ScriptInstrumentor + +class MockRunningScript: + instance = None + + def __init__(self): + MockRunningScript.instance = self + self.pre_lines = [] + self.post_lines = [] + self.exceptions = [] + + def pre_line_instrumentation(self, filename, lineno, globals, locals): + self.pre_lines.append((filename, lineno)) + + def post_line_instrumentation(self, filename, lineno): + self.post_lines.append((filename, lineno)) + + def exception_instrumentation(self, filename, lineno): + self.exceptions.append((filename, lineno)) + return False + +@pytest.fixture +def mock_running_script(): + return MockRunningScript() + +def test_simple_script(mock_running_script): + script = """ +x = 1 +y = 2 +z = x + y +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [] + +def test_try_except_script(mock_running_script): + script = """ +try: + x = 1 / 0 +except ZeroDivisionError: + x = 0 +print('done') +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 5), + ("testfile.py", 6), + ] + assert mock_running_script.post_lines == [ + # Line 2 doesn't exit because it raises an exception + ("testfile.py", 3), + ("testfile.py", 5), + ("testfile.py", 6), + ] + assert mock_running_script.exceptions == [] + +def test_if_else_script(mock_running_script): + script = """ +x = 1 +if x == 1: + y = 2 +else: + y = 3 +z = x + y +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ("testfile.py", 7), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 4), + ("testfile.py", 7), + ] + assert mock_running_script.exceptions == [] + +def test_for_loop_script(mock_running_script): + script = """ +for i in range(3): + print(i) +print('done') +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [] + +def test_match_script(mock_running_script): + script = """ +x = "HI" +match x: + case "HI": + print(x) + case "NO": + print(x) +print('done') +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + print(ast.dump(tree, indent=4)) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), # x = "HI" + ("testfile.py", 3), # match x: + # We can't match the case statement because it must come after the match statement + ("testfile.py", 5), # print(x) + ("testfile.py", 8), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 5), + ("testfile.py", 8), + ] + assert mock_running_script.exceptions == [] + +def test_exception_script(mock_running_script): + script = """ +x = "HI" +raise RuntimeError("Error") +print('done') +""" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + print(ast.dump(tree, indent=4)) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [ + ("testfile.py", 3), + ] From 56fdc7b2e130c1274d1732e16403613c4121867e Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 21 Jan 2025 11:12:46 -0700 Subject: [PATCH 2/5] Fix linter errors --- .github/workflows/api_tests.yml | 5 ++-- .../scripts/run_script.py | 4 +-- .../scripts/running_script.py | 25 +++++++++---------- .../scripts/script_instrumentor.py | 1 + 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/.github/workflows/api_tests.yml b/.github/workflows/api_tests.yml index 310b113c2a..eea5da5c75 100644 --- a/.github/workflows/api_tests.yml +++ b/.github/workflows/api_tests.yml @@ -107,11 +107,12 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip + pip install -r requirements.txt pip install -r requirements-dev.txt - working-directory: openc3-cosmos-script-runner-api + working-directory: openc3/python - name: Lint with ruff run: | - ruff --format=github scripts/*.py + ruff --config=../openc3/python/pyproject.toml --format=github scripts/*.py working-directory: openc3-cosmos-script-runner-api - name: Run unit tests run: | diff --git a/openc3-cosmos-script-runner-api/scripts/run_script.py b/openc3-cosmos-script-runner-api/scripts/run_script.py index 0f2f2e0c36..fc5ab85418 100644 --- a/openc3-cosmos-script-runner-api/scripts/run_script.py +++ b/openc3-cosmos-script-runner-api/scripts/run_script.py @@ -164,7 +164,7 @@ def run_script_log(id, message, color="BLACK", message_log=True): | "open_file_dialog" | "open_files_dialog" ): - if running_script.prompt_id != None: + if running_script.prompt_id is not None: if ( "prompt_id" in parsed_cmd and running_script.prompt_id @@ -246,7 +246,7 @@ def run_script_log(id, message, color="BLACK", message_log=True): run_script_log( id, f"ERROR: Script command not handled: {msg['data']}", "RED" ) -except Exception as err: +except Exception: tb = traceback.format_exc() run_script_log(id, tb, "RED") finally: diff --git a/openc3-cosmos-script-runner-api/scripts/running_script.py b/openc3-cosmos-script-runner-api/scripts/running_script.py index 603a5ed49c..98a9338014 100644 --- a/openc3-cosmos-script-runner-api/scripts/running_script.py +++ b/openc3-cosmos-script-runner-api/scripts/running_script.py @@ -18,7 +18,6 @@ from openc3.utilities.string import build_timestamped_filename from openc3.utilities.bucket_utilities import BucketUtilities from openc3.script.storage import _get_storage_file -import re import linecache @@ -435,7 +434,11 @@ def unique_filename(self): return "Untitled" + str(RunningScript.id) def stop_message_log(self): - metadata = {"id": self.id, "user": self.details["user"], "scriptname": self.unique_filename()} + metadata = { + "id": self.id, + "user": self.details["user"], + "scriptname": self.unique_filename(), + } if RunningScript.my_message_log: RunningScript.my_message_log.stop(True, metadata=metadata) RunningScript.my_message_log = None @@ -448,7 +451,7 @@ def set_filename(self, filename): # Deal with breakpoints created under the previous filename. bkpt_filename = self.unique_filename() - if not bkpt_filename in RunningScript.breakpoints: + if bkpt_filename not in RunningScript.breakpoints: RunningScript.breakpoints[bkpt_filename] = RunningScript.breakpoints[ self.filename ] @@ -539,7 +542,7 @@ def exception_instrumentation(self, filename, line_number): if ( exc_type == StopScript or exc_type == SkipScript - or exc_type == SkipTestCase # DEPRECATED but still valid + or exc_type == SkipTestCase # DEPRECATED but still valid or not self.use_instrumentation ): raise exc_value @@ -597,20 +600,20 @@ def debug(self, debug_text): @classmethod def set_breakpoint(cls, filename, line_number): - if not filename in cls.breakpoints: + if filename not in cls.breakpoints: cls.breakpoints[filename] = {} cls.breakpoints[filename][line_number] = True @classmethod def clear_breakpoint(cls, filename, line_number): - if not filename in cls.breakpoints: + if filename not in cls.breakpoints: cls.breakpoints[filename] = {} if line_number in cls.breakpoints[filename]: del cls.breakpoints[filename][line_number] @classmethod def clear_breakpoints(cls, filename=None): - if filename == None or filename == "": + if filename is None or filename == "": cls.breakpoints = {} else: if filename in cls.breakpoints: @@ -679,7 +682,7 @@ def handle_output_io(self, filename=None, line_number=None): out_line = json_hash["log"] if "message" in json_hash: out_line = json_hash["message"] - except: + except Exception: # Regular output pass @@ -722,10 +725,6 @@ def handle_output_io(self, filename=None, line_number=None): # Add to the message log self.message_log().write(lines_to_write) - def graceful_kill(self): - # Just to avoid warning - pass - def wait_for_go_or_stop(self, error=None, prompt=None): count = -1 self.go = False @@ -1098,7 +1097,7 @@ def run_text( def handle_potential_tab_change(self, filename): # Make sure the correct file is shown in script runner if self.current_file != filename: - if not filename in self.call_stack: + if filename not in self.call_stack: self.call_stack.append(filename) self.load_file_into_script(filename) self.current_file = filename diff --git a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py index 5f71e177d8..91649812ab 100644 --- a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py @@ -20,6 +20,7 @@ # For details on the AST, see https://docs.python.org/3/library/ast.html # and https://greentreesnakes.readthedocs.io/en/latest/nodes.html + # This class is used to instrument a Python script with calls to a # RunningScript instance. The RunningScript instance is used to # track the execution of the script, and can be used to pause and From e71e445fa487259a4cc60aa67b3a552eb419f1df Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 21 Jan 2025 11:28:16 -0700 Subject: [PATCH 3/5] Fix script_isntrumentor to work with python 3.10 --- .../scripts/script_instrumentor.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py index 91649812ab..559456509a 100644 --- a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py @@ -15,6 +15,7 @@ # if purchased from OpenC3, Inc. import ast +import sys # For details on the AST, see https://docs.python.org/3/library/ast.html @@ -47,6 +48,9 @@ class ScriptInstrumentor(ast.NodeTransformer): def __init__(self, filename): self.filename = filename self.in_try = False + self.try_nodes = [ast.Try] + if sys.version_info >= (3, 11): + self.try_nodes.append(ast.TryStar) # What we're trying to do is wrap executable statements in a while True try/except block # For example if the input code is "print('HI')", we want to transform it to: @@ -67,11 +71,11 @@ def __init__(self, filename): def track_enter_leave(self, node): # Determine if we're in a try block in_try = self.in_try - if not in_try and type(node) in (ast.Try, ast.TryStar): + if not in_try and type(node) in self.try_nodes: self.in_try = True # Visit the children of the node node = self.generic_visit(node) - if not in_try and type(node) in (ast.Try, ast.TryStar): + if not in_try and type(node) in self.try_nodes: self.in_try = False # ast.parse returns a module, so we need to extract # the first element of the body which is the node @@ -129,11 +133,11 @@ def track_enter_leave(self, node): ast.copy_location(try_node, node) return try_node - # Call the pre_line_instrumentation ONLY and then exceute the node + # Call the pre_line_instrumentation ONLY and then execute the node def track_reached(self, node): # Determine if we're in a try block, this is used by track_enter_leave in_try = self.in_try - if not in_try and type(node) in (ast.Try, ast.TryStar): + if not in_try and type(node) in self.try_nodes: self.in_try = True # Visit the children of the node @@ -178,7 +182,8 @@ def track_reached(self, node): visit_Raise = track_enter_leave visit_Try = track_reached - visit_TryStar = track_reached + if sys.version_info >= (3, 11): + visit_TryStar = track_reached visit_Assert = track_enter_leave visit_Import = track_enter_leave From 3cf537444ba9ababee06c8a20f7148ae33443e2c Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 21 Jan 2025 15:25:55 -0700 Subject: [PATCH 4/5] Reformat test code --- .../test/scripts/test_script_instrumentor.py | 264 +++++++++--------- 1 file changed, 136 insertions(+), 128 deletions(-) diff --git a/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py index 13e9d64715..8ed76c87fe 100644 --- a/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py @@ -2,81 +2,86 @@ import pytest from scripts.script_instrumentor import ScriptInstrumentor + class MockRunningScript: - instance = None + instance = None + + def __init__(self): + MockRunningScript.instance = self + self.pre_lines = [] + self.post_lines = [] + self.exceptions = [] - def __init__(self): - MockRunningScript.instance = self - self.pre_lines = [] - self.post_lines = [] - self.exceptions = [] + def pre_line_instrumentation(self, filename, lineno, globals, locals): + self.pre_lines.append((filename, lineno)) - def pre_line_instrumentation(self, filename, lineno, globals, locals): - self.pre_lines.append((filename, lineno)) + def post_line_instrumentation(self, filename, lineno): + self.post_lines.append((filename, lineno)) - def post_line_instrumentation(self, filename, lineno): - self.post_lines.append((filename, lineno)) + def exception_instrumentation(self, filename, lineno): + self.exceptions.append((filename, lineno)) + return False - def exception_instrumentation(self, filename, lineno): - self.exceptions.append((filename, lineno)) - return False @pytest.fixture def mock_running_script(): - return MockRunningScript() + return MockRunningScript() + def test_simple_script(mock_running_script): - script = """ + script = """ x = 1 y = 2 z = x + y """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.post_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.exceptions == [] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [] + def test_try_except_script(mock_running_script): - script = """ + script = """ try: x = 1 / 0 except ZeroDivisionError: x = 0 print('done') """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 5), - ("testfile.py", 6), - ] - assert mock_running_script.post_lines == [ - # Line 2 doesn't exit because it raises an exception - ("testfile.py", 3), - ("testfile.py", 5), - ("testfile.py", 6), - ] - assert mock_running_script.exceptions == [] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 5), + ("testfile.py", 6), + ] + assert mock_running_script.post_lines == [ + # Line 2 doesn't exit because it raises an exception + ("testfile.py", 3), + ("testfile.py", 5), + ("testfile.py", 6), + ] + assert mock_running_script.exceptions == [] + def test_if_else_script(mock_running_script): - script = """ + script = """ x = 1 if x == 1: y = 2 @@ -84,52 +89,54 @@ def test_if_else_script(mock_running_script): y = 3 z = x + y """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 4), - ("testfile.py", 7), - ] - assert mock_running_script.post_lines == [ - ("testfile.py", 2), - ("testfile.py", 4), - ("testfile.py", 7), - ] - assert mock_running_script.exceptions == [] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ("testfile.py", 7), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 4), + ("testfile.py", 7), + ] + assert mock_running_script.exceptions == [] + def test_for_loop_script(mock_running_script): - script = """ + script = """ for i in range(3): print(i) print('done') """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 3), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.post_lines == [ - ("testfile.py", 3), - ("testfile.py", 3), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.exceptions == [] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [] + def test_match_script(mock_running_script): - script = """ + script = """ x = "HI" match x: case "HI": @@ -138,48 +145,49 @@ def test_match_script(mock_running_script): print(x) print('done') """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - print(ast.dump(tree, indent=4)) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), # x = "HI" - ("testfile.py", 3), # match x: - # We can't match the case statement because it must come after the match statement - ("testfile.py", 5), # print(x) - ("testfile.py", 8), - ] - assert mock_running_script.post_lines == [ - ("testfile.py", 2), - ("testfile.py", 5), - ("testfile.py", 8), - ] - assert mock_running_script.exceptions == [] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + print(ast.dump(tree, indent=4)) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), # x = "HI" + ("testfile.py", 3), # match x: + # We can't match the case statement because it must come after the match statement + ("testfile.py", 5), # print(x) + ("testfile.py", 8), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 5), + ("testfile.py", 8), + ] + assert mock_running_script.exceptions == [] + def test_exception_script(mock_running_script): - script = """ + script = """ x = "HI" raise RuntimeError("Error") print('done') """ - parsed = ast.parse(script) - tree = ScriptInstrumentor("testfile.py").visit(parsed) - print(ast.dump(tree, indent=4)) - compiled = compile(tree, filename="testfile.py", mode="exec") - exec(compiled, {"RunningScript": mock_running_script}) - - assert mock_running_script.pre_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.post_lines == [ - ("testfile.py", 2), - ("testfile.py", 3), - ("testfile.py", 4), - ] - assert mock_running_script.exceptions == [ - ("testfile.py", 3), - ] + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + print(ast.dump(tree, indent=4)) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ("testfile.py", 3), + ("testfile.py", 4), + ] + assert mock_running_script.exceptions == [ + ("testfile.py", 3), + ] From 39d5d943c934b2bb524f2759043b11e02dff3a45 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 21 Jan 2025 17:16:14 -0700 Subject: [PATCH 5/5] Add test for from __future__ import --- .../scripts/script_instrumentor.py | 8 +++++++- .../test/scripts/test_script_instrumentor.py | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py index 559456509a..5ce37ac873 100644 --- a/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/scripts/script_instrumentor.py @@ -155,6 +155,12 @@ def track_reached(self, node): ast.copy_location(if_node, node) return if_node + def track_import_from(self, node): + # Don't tract from __future__ imports because they must come first or: + # SyntaxError: from __future__ imports must occur at the beginning of the file + if node.module != '__future__': + return self.track_enter_leave(node) + # Notes organized (including newlines) per https://docs.python.org/3/library/ast.html#abstract-grammar # Nodes that change control flow are processed by track_reached, otherwise we track_enter_leave visit_FunctionDef = track_reached @@ -187,7 +193,7 @@ def track_reached(self, node): visit_Assert = track_enter_leave visit_Import = track_enter_leave - visit_ImportFrom = track_enter_leave + visit_ImportFrom = track_import_from visit_Global = track_enter_leave visit_Nonlocal = track_enter_leave diff --git a/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py index 8ed76c87fe..13fe8e7b45 100644 --- a/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py +++ b/openc3-cosmos-script-runner-api/test/scripts/test_script_instrumentor.py @@ -147,7 +147,6 @@ def test_match_script(mock_running_script): """ parsed = ast.parse(script) tree = ScriptInstrumentor("testfile.py").visit(parsed) - print(ast.dump(tree, indent=4)) compiled = compile(tree, filename="testfile.py", mode="exec") exec(compiled, {"RunningScript": mock_running_script}) @@ -174,7 +173,6 @@ def test_exception_script(mock_running_script): """ parsed = ast.parse(script) tree = ScriptInstrumentor("testfile.py").visit(parsed) - print(ast.dump(tree, indent=4)) compiled = compile(tree, filename="testfile.py", mode="exec") exec(compiled, {"RunningScript": mock_running_script}) @@ -191,3 +189,19 @@ def test_exception_script(mock_running_script): assert mock_running_script.exceptions == [ ("testfile.py", 3), ] + + +def test_import_future_script(mock_running_script): + script = "from __future__ import annotations\nprint('hi')" + parsed = ast.parse(script) + tree = ScriptInstrumentor("testfile.py").visit(parsed) + compiled = compile(tree, filename="testfile.py", mode="exec") + exec(compiled, {"RunningScript": mock_running_script}) + + assert mock_running_script.pre_lines == [ + ("testfile.py", 2), + ] + assert mock_running_script.post_lines == [ + ("testfile.py", 2), + ] + assert mock_running_script.exceptions == []