Add a new checker to check cross-feature dependency. #15559
+311
−0
Conversation
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
yutongzhang-microsoft
force-pushed
the
yutongzhang/check_dependence
branch
4 times, most recently
from
d0f622e
to
752d893
Compare
xwjiang-ms
reviewed
Nov 15, 2024
| continueOnError: true | ||
| pool: sonic-common | ||
| steps: | ||
| - script: | |
There was a problem hiding this comment.
suggest to use a template yml here to make azure-pipelines.yml clean
There was a problem hiding this comment.
There is only one step here, so I think there is no necessary to create another template file here?
xwjiang-ms
reviewed
Nov 15, 2024
| 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"])) |
There was a problem hiding this comment.
Here may have duplicate prints, how about just print 1 line and continue, or you can also print the alias
There was a problem hiding this comment.
I think one script one line is more clearly for user to read.
yutongzhang-microsoft
force-pushed
the
yutongzhang/check_dependence
branch
from
2b725a9
to
8d6807f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
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.
In the tests directory of the
sonic-mgmtrepository, we have categorized scripts into two main groups: shared scripts and feature-specific scripts. Shared scripts provide common utilities or functionality used across multiple features, while feature-specific scripts are tied to particular features and their corresponding logic.However, the previous codebase contained a significant number of cross-feature dependencies, where scripts from one feature directly referenced or relied on scripts from another. To address this issue and align with our new testing model, we manually reviewed the existing code and removed all cross-feature references. But we need a mechanism to check future modifications and new code to prevent reintroducing these issues.
In this PR, we introduce a new checker to identify any cross-feature dependencies. At this stage, since some cross-feature dependencies remain in the code, this checker is configured to flag these dependencies without causing the entire test to fail. Once all current dependencies are fully removed, any reintroduced cross-feature dependencies detected by this checker will result in a test failure.
Summary:
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
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.
In the tests directory of the
sonic-mgmtrepository, we have categorized scripts into two main groups: shared scripts and feature-specific scripts. Shared scripts provide common utilities or functionality used across multiple features, while feature-specific scripts are tied to particular features and their corresponding logic.However, the previous codebase contained a significant number of cross-feature dependencies, where scripts from one feature directly referenced or relied on scripts from another. To address this issue and align with our new testing model, we manually reviewed the existing code and removed all cross-feature references. But we need a mechanism to check future modifications and new code to prevent reintroducing these issues.
In this PR, we introduce a new checker to identify any cross-feature dependencies. At this stage, since some cross-feature dependencies remain in the code, this checker is configured to flag these dependencies without causing the entire test to fail. Once all current dependencies are fully removed, any reintroduced cross-feature dependencies detected by this checker will result in a test failure.
How did you do it?
In this PR, we introduce a new checker to identify any cross-feature dependencies. At this stage, since some cross-feature dependencies remain in the code, this checker is configured to flag these dependencies without causing the entire test to fail. Once all current dependencies are fully removed, any reintroduced cross-feature dependencies detected by this checker will result in a test failure.
How did you verify/test it?
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation