forked from sonic-net/sonic-mgmt
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge branch 'sonic-net:master' into master
- Loading branch information
Showing
390 changed files
with
60,910 additions
and
5,488 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
steps: | ||
- script: | | ||
set -x | ||
pip3 install natsort | ||
CHECK_RESULT=$(python3 ./.azure-pipelines/dependency_check/dependency_check.py tests) | ||
if [[ "$CHECK_RESULT" == "True" ]]; then | ||
echo "##vso[task.complete result=Failed;]Condition check failed." | ||
exit 1 | ||
fi | ||
displayName: "Dependency Check" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
## Background | ||
We introduced a new approach to PR testing called _Impacted area based PR testing_. \ | ||
In this model, the scope of PR testing is determined by the specific areas of the code that are impacted by the changes, | ||
allowing for more focused and efficient testing. | ||
This means, we need to establish clear boundaries between different sections of code | ||
and minimize dependencies as much as possible. | ||
|
||
We can consider the test scripts in this way: | ||
``` | ||
sonic-mgmgt | ||
| | ||
| - tests | ||
| | ||
| - common ---------- shared | ||
| - arp -----| | ||
| - ecmp | --- features | ||
| - vlan | | ||
| - ...... -----| | ||
``` | ||
Within the tests directory in sonic-mgmt, we categorize scripts into two sections: shared and features. | ||
Scripts in the common folder fall under the shared section and can be utilized across different folders. | ||
In contrast, scripts in other folders belong to the features section, representing specific functionalities such as arp, ecmp, and vlan, | ||
and are intended for use within their respective folders. | ||
|
||
However, the previous code had numerous cross-feature dependencies. | ||
To achieve the above goal, we have removed the cross-feature references from the existing code. | ||
But we need a mechanism to check future modifications and new code to prevent reintroducing these issues. | ||
|
||
|
||
## Design | ||
The _ast_ module helps python applications to process trees of the python abstract syntax grammar. | ||
This module produces a tree of objects, where each object is an instance of a class that inherits from _ast.AST_. | ||
There are two classes related to the imports: | ||
|
||
#### ast.Import | ||
- An import statement such as `import x as a,y` | ||
- _names_ is a list of alias nodes. | ||
``` | ||
Import(names=[ | ||
alias(name='x', | ||
asname='a') | ||
]), | ||
Import(names=[ | ||
alias(name='y', | ||
asname=None) | ||
]), | ||
``` | ||
#### ast.ImportFrom | ||
- Represents `from x import y,z`. | ||
- _module_ is a raw string of the ‘from’ name, without any leading dots, or None for statements such as `from . import foo.` | ||
- _level_ is an integer holding the level of the relative import (0 means absolute import) | ||
``` | ||
ImportFrom( | ||
module='x', | ||
names=[ | ||
alias(name='y', asname=None), | ||
alias(name='z', asname=None)], | ||
level=0) | ||
``` | ||
|
||
To achieve our goal, we need to follow these steps. | ||
+ Gather all scripts to be analyzed | ||
+ Identify all imported modules in each script along with their import paths | ||
+ Compare each imported path with its corresponding script path | ||
|
||
### Gather all scripts to be analyzed | ||
To collect all scripts for analysis, | ||
we can use `os.walk` to gather every script within the specified path | ||
|
||
### Identify all imported modules in each script along with their import paths | ||
To identify all imported modules, | ||
we can use the _ast_ module, as mentioned above, to analyze each collected script and obtain its abstract syntax tree. | ||
Then, using the _ast.ImportFrom_ and _ast.Import_ classes, we can extract the imported modules from each script. | ||
|
||
|
||
Here are the steps and configuration methods for Python to search for module paths: | ||
+ The current script's directory or the directory from which the Python interpreter is started. | ||
+ Standard library path: Contains the standard library modules from the Python installation directory. | ||
+ Third-party library path: For example, the site-packages directory, where third-party libraries installed via pip and other tools are stored. | ||
+ Environment variable path: Custom directories can be added to sys.path via the PYTHONPATH environment variable. | ||
|
||
As paths of project is not included in the sys path, we need to add them into sys path first. | ||
|
||
+ `importlib.util.find_spec` is a function in Python that is used to find the specification of a module. | ||
The specification contains details about the module, such as its location (file path), loader, and other attributes. | ||
It can find the path of standard library, third-party libraries and custom modules which are imported with no hierarchy. | ||
|
||
For statement like `import math`, `from tests.common.plugins.allure_wrapper import allure_step_wrapper`, `from gnmi_utils import apply_gnmi_file`, | ||
we can use `importlib.util.find_spec` to get their imported path. | ||
+ For hierarchy imported, we can calculate the abs path using the current file path and level to navigate up to the corresponding directory. | ||
|
||
### Compare each imported path with its corresponding script path | ||
We will focus only on imported paths that start with `sonic-mgmt/tests`. | ||
Paths imported from other folders within `sonic-mgmt` are treated as common locations. | ||
|
||
For paths beginning with `sonic-mgmt/tests`, there are three special cases: | ||
+ sonic-mgmt/tests/common | ||
+ sonic-mgmt/tests/ptf_runner.py | ||
+ sonic-mgmt/tests/conftest.py | ||
which are also considered as common paths. | ||
|
||
For all other paths, we will compare each imported path to the path of the corresponding script based on the following principles: | ||
+ The first-level folders under `sonic-mgmt/tests` (e.g., arp, bgp) are considered feature folders. | ||
+ If both the imported module and the script are in the same feature folder, there is no cross-feature dependency. | ||
+ If they are in different feature folders, it indicates a cross-feature dependency, causing the check to fail. | ||
|
||
|
||
We will add this check as a step in `Pre_test` in PR test. |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,219 @@ | ||
import ast | ||
import sys | ||
import os | ||
import importlib.util | ||
from natsort import natsorted | ||
from contextlib import contextmanager | ||
|
||
|
||
def collect_all_scripts(): | ||
""" | ||
Recursively find all files ending with ".py" under the folder "tests" | ||
Note: The full path and name of files are stored in a list named "files" | ||
Returns: | ||
A list of files ending with ".py" under the folder "tests" | ||
""" | ||
location = sys.argv[1] | ||
files = [] | ||
for root, dirs, file in os.walk(location): | ||
for f in file: | ||
if f.endswith(".py"): | ||
files.append(os.path.join(root, f)) | ||
files = natsorted(files) | ||
return files | ||
|
||
|
||
@contextmanager | ||
def set_sys_path(file_path): | ||
""" | ||
Add all the paths related to the file into sys path | ||
Args: | ||
file_path (list): A list of files ending with ".py" under the folder "tests" | ||
Returns: | ||
None | ||
""" | ||
original_sys_path = sys.path.copy() | ||
try: | ||
current_dir = os.path.abspath(os.path.dirname(file_path)) | ||
while current_dir != os.path.dirname(current_dir): | ||
if current_dir.endswith("/tests"): | ||
sys.path.append(os.path.join(current_dir, "common")) | ||
|
||
sys.path.append(current_dir) | ||
current_dir = os.path.dirname(current_dir) | ||
yield | ||
finally: | ||
sys.path = original_sys_path | ||
|
||
|
||
def get_module_path(imported_module, level=0, file_path=""): | ||
""" | ||
Get the abs path of the imported module | ||
Args: | ||
imported_module (string): The imported module imported in the script. | ||
level (int): The import level that generated by ast. | ||
file_path (string): The path of a test script. | ||
Returns: | ||
string/None: The absolute path of the imported module or None | ||
""" | ||
try: | ||
if level == 0: | ||
# Level 0 means an absolute import. | ||
# This means that the import statement is intended to refer directly | ||
# to the module or package path as specified without any relative hierarchy. | ||
# So we can get the module path using "importlib.util.find_spec" | ||
spec = importlib.util.find_spec(imported_module) | ||
if spec and spec.origin: | ||
return spec.origin | ||
if level == 1: | ||
# Level 1 means the import is relative to the current package level, | ||
# so the module path shares the same dirname with the file. | ||
# To save time, we don't need to check such import module. | ||
return None | ||
else: | ||
# For level which is higher than 1, | ||
# the number represents how many levels up in the package hierarchy the import should go. | ||
# Based on the current file path and the specified level, we can navigate up to the corresponding directory | ||
# and then combine the module name with the upper-level path to form an absolute path | ||
base_dir = os.path.abspath(file_path) | ||
for _ in range(level): | ||
base_dir = os.path.dirname(base_dir) | ||
return os.path.join(base_dir, *imported_module.split(".")) | ||
except ModuleNotFoundError: | ||
return None | ||
|
||
|
||
def get_imported_modules(files): | ||
""" | ||
Get all imported modules in each file. | ||
Args: | ||
files (list): A list of files ending with ".py" under the folder "tests" | ||
Returns: | ||
dict: All imported modules in test scripts. The output formatted as below | ||
{ | ||
'../tests/acl/custom_acl_table/test_custom_acl_table.py': [ | ||
{ | ||
'type': 'from_import', | ||
'module': 'ptf.mask', | ||
'module_path': '/usr/local/lib/python3.8/dist-packages/ptf/mask.py', | ||
'alias': 'Mask', | ||
'asname': None | ||
}, | ||
{ | ||
'type': 'from_import', | ||
'module': 'tests.common.fixtures.ptfhost_utils', | ||
'module_path': '/data/sonic-mgmt/tests/common/fixtures/ptfhost_utils.py', | ||
'alias': 'skip_traffic_test', | ||
'asname': None | ||
} | ||
], | ||
'../tests/bgp/test_bgp_session_flap.py': [ | ||
{ | ||
'type': 'from_import', | ||
'module': 'tests.common.utilities', | ||
'module_path': '/data/sonic-mgmt/tests/common/utilities.py', | ||
'alias': 'InterruptableThread', | ||
'asname': None | ||
} | ||
] | ||
} | ||
""" | ||
imported_modules_in_files = {} | ||
for file_path in files: | ||
# For each file, we need to add its related path into sys path | ||
with set_sys_path(file_path): | ||
# We use ast to analyse the file as an abstract syntax tree, | ||
# and get all imported modules using class `ast.Import` and `ast.ImportFrom` | ||
with open(file_path, "r", encoding="utf-8") as file: | ||
tree = ast.parse(file.read(), filename=file_path) | ||
imported_modules_in_files[file_path] = [] | ||
for node in ast.walk(tree): | ||
# Check for `import` statements | ||
if isinstance(node, ast.Import): | ||
for entry in node.names: | ||
imported_modules_in_files[file_path].append({ | ||
"type": "import", | ||
"module": entry.name, | ||
"module_path": get_module_path(entry.name), | ||
"asname": entry.asname | ||
}) | ||
# Check for `from ... import ...` statements | ||
if isinstance(node, ast.ImportFrom): | ||
for entry in node.names: | ||
imported_modules_in_files[file_path].append({ | ||
"type": "from_import", | ||
"module": node.module, | ||
"module_path": get_module_path(node.module, node.level, file_path), | ||
"alias": entry.name, | ||
"asname": entry.asname | ||
}) | ||
return imported_modules_in_files | ||
|
||
|
||
def get_feature_path(path): | ||
""" | ||
For our repo, we can consider the folders like "acl", "bgp" as feature folders. | ||
In this function, we will retrieve the path of the top-level feature directories. | ||
In other words, we will retrieve the absolute paths of the first-level folders under `sonic-mgmt/tests` | ||
Args: | ||
path (string): The path of a file or an import module | ||
Returns: | ||
string/None: The absolute feature path or None | ||
""" | ||
if path is None: | ||
return None | ||
|
||
file_path = os.path.abspath(path) | ||
target_path = "tests" | ||
index = file_path.find(target_path) | ||
|
||
if index != -1: | ||
project_path = file_path[:index + len(target_path)] | ||
else: | ||
return None | ||
|
||
feature = file_path[len(project_path) + 1:].split("/")[0] | ||
return os.path.join(project_path, feature) | ||
|
||
|
||
def check_cross_dependency(imports_in_script): | ||
""" | ||
Check if there are cross-feature dependency in each file. | ||
Args: | ||
imports_in_script (dict): All imported modules in test scripts. | ||
Returns: | ||
bool: True is there are cross-feature dependencies and False is there is no cross-feature dependencies | ||
""" | ||
cross_dependency = False | ||
for file_path, imported_modules in imports_in_script.items(): | ||
file_feature_path = get_feature_path(file_path) | ||
for imported_module in imported_modules: | ||
imported_module_feature_path = get_feature_path(imported_module["module_path"]) | ||
if imported_module_feature_path is not None: | ||
project_path = os.path.dirname(file_feature_path) | ||
# Import from these paths are allowed. | ||
if imported_module_feature_path not in [os.path.join(project_path, "common"), | ||
os.path.join(project_path, "ptf_runner.py"), | ||
os.path.join(project_path, "conftest.py"), | ||
file_feature_path]: | ||
print("There is a cross-feature dependence. File: {}, import module: {}" | ||
.format(file_path, imported_module["module"])) | ||
cross_dependency = True | ||
print(cross_dependency) | ||
return cross_dependency | ||
|
||
|
||
if __name__ == '__main__': | ||
files = collect_all_scripts() | ||
imported_modules_in_files = get_imported_modules(files) | ||
cross_dependency = check_cross_dependency(imported_modules_in_files) | ||
if cross_dependency: | ||
print("\033[31mThere are cross-feature dependencies, which is not allowed in our repo\033[0m") | ||
print("\033[31mTo resolve this issue, please move the shared function to common place, " | ||
"such as 'tests/common'\033[0m") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.