Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run yang validation in unit test #3025

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Oct 24, 2023

What I did

Add unit test for db_migrator.py: ConfigDB passing yang validation.
Microsoft ADO: 24657445
Pending on sonic-net/sonic-buildimage#16974

How I did it

Add yang validation for unit test, and fix test data to pass yang validation.

How to verify it

Run sonic-utilities end to end test.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@@ -3,17 +3,17 @@
"VERSION": "version_4_0_3"
},
"FLEX_COUNTER_TABLE|ACL": {
"FLEX_COUNTER_STATUS": "true",
"FLEX_COUNTER_STATUS": "enable",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enable

Just wondering, should we fix yang model, or fix the test data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check with owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vadymhlushko-mlnx has confirmed this is a mistake.

@@ -1122,6 +1124,27 @@ def migrate(self):
version = next_version
# Perform common migration ops
self.common_migration_ops()
if os.environ["UTILITIES_UNIT_TESTING"] == "2":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTILITIES_UNIT_TESTING

I am thinking this detection is useful in production runtime as background task, not just unit test. It will help detect bugs like:

  1. db_migrator generates a new config but not yang validated
  2. load_minigraph generates a new config but not yang validated

Not sure if there are other places the yang validation could help in runtime. But let's only detect and generate CRIT syslog, not to block the existing workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@ganglyu
Copy link
Contributor Author

ganglyu commented Oct 28, 2023

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ganglyu
Copy link
Contributor Author

ganglyu commented Oct 28, 2023

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wen587
Copy link
Contributor

wen587 commented Nov 1, 2023

I saw this PR is pending on https://github.com/sonic-net/sonic-buildimage/pull/16974/files.
Just a reminder in another ADO #25626334, we are trying to use just string instead of pattern as there are many device type are not covered in this yang model: sonic-device_neighbor_metadata.yang

Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ganglyu ganglyu merged commit e01fc89 into sonic-net:master Nov 1, 2023
5 checks passed
yxieca added a commit that referenced this pull request Nov 29, 2023
yxieca added a commit that referenced this pull request Nov 30, 2023
yxieca added a commit that referenced this pull request Dec 14, 2023
ganglyu added a commit that referenced this pull request Dec 18, 2023
@ganglyu ganglyu deleted the yang_validation branch December 27, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants