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

[sonic-cfggen]: Update UT to run yang validation #9700

Merged
merged 21 commits into from
May 16, 2022

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Jan 7, 2022

Signed-off-by: Gang Lv [email protected]

Why I did it

Config db schema generated by minigraph should run yang validation.

How I did it

Modify run_script to add yang validation.

How to verify it

Run sonic-config-engine unit test.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@ganglyu ganglyu added sonic-cfggen SONiC Configuration Generator Tool YANG YANG model related changes labels Jan 7, 2022
@qiluo-msft
Copy link
Collaborator

@judyjoseph Could you check this unit test improvement also working for multi-ASIC?

cmd += ' -n ' + args.namespace
cmd += ' --print-data'
output = subprocess.check_output(cmd, shell=True).decode()
self.yang_parser.loadData(configdbJson=json.loads(output))
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can throw, place it in the try catch.

@qiluo-msft qiluo-msft added the Request for 202111 Branch For PRs being requested for 202111 branch label Jan 13, 2022
qiluo-msft
qiluo-msft previously approved these changes Jan 13, 2022
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please check with other reviewers.

Signed-off-by: Gang Lv [email protected]
Signed-off-by: Gang Lv [email protected]
self.yang_parser.validate_data_tree()
except sonic_yang.SonicYangException as e:
print("yang data generated from %s is not valid"%(args.minigraph))
raise
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 19, 2022

Choose a reason for hiding this comment

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

raise

raise is not friendly in unit test, You may return a bool, and a string. And the caller could use assert to apply the pair.

Validation failure should be treated as a test case failure, not an exception. #Closed

@ganglyu
Copy link
Contributor Author

ganglyu commented Apr 22, 2022

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zhangyanzhao
Copy link
Collaborator

Still has one blocking issue that MSFT team is working on.

@zhangyanzhao
Copy link
Collaborator

Still one more blocking issue and MSFT team is working on that issue.

@zhangyanzhao
Copy link
Collaborator

This is addressed by #10786 and close this PR.

@qiluo-msft
Copy link
Collaborator

This PR is still open.

@qiluo-msft qiluo-msft reopened this May 12, 2022
@ganglyu ganglyu requested a review from qiluo-msft May 15, 2022 12:35
@ganglyu ganglyu merged commit 1db50e5 into sonic-net:master May 16, 2022
judyjoseph pushed a commit that referenced this pull request May 16, 2022
Why I did it
Config db schema generated by minigraph should run yang validation.

How I did it
Modify run_script to add yang validation.

How to verify it
Run sonic-config-engine unit test.

Signed-off-by: Gang Lv [email protected]
@stephenxs
Copy link
Collaborator

@judyjoseph @qiluo-msft
Looks like cherry-picking this PR to 202111 fails the build test. Can you please check whether we miss something other PRs?

2022-05-17T04:17:53.5225444Z ======================================================================
2022-05-17T04:17:53.5226298Z FAIL: test_minigraph_voq_metadata (tests.test_cfggen.TestCfgGen)
2022-05-17T04:17:53.5227843Z ----------------------------------------------------------------------
2022-05-17T04:17:53.5228597Z Traceback (most recent call last):
2022-05-17T04:17:53.5229846Z   File "/sonic/src/sonic-config-engine/tests/test_cfggen.py", line 827, in test_minigraph_voq_metadata
2022-05-17T04:17:53.5230781Z     output = json.loads(self.run_script(argument))
2022-05-17T04:17:53.5232119Z   File "/sonic/src/sonic-config-engine/tests/test_cfggen.py", line 54, in run_script
2022-05-17T04:17:53.5232954Z     self.assertTrue(self.yang.validate(argument))
2022-05-17T04:17:53.5233700Z AssertionError: False is not true
2022-05-17T04:17:53.5233988Z 
2022-05-17T04:17:53.5234579Z ======================================================================
2022-05-17T04:17:53.5235398Z FAIL: test_minigraph_voq_system_ports (tests.test_cfggen.TestCfgGen)
2022-05-17T04:17:53.5236814Z ----------------------------------------------------------------------
2022-05-17T04:17:53.5237538Z Traceback (most recent call last):
2022-05-17T04:17:53.5238882Z   File "/sonic/src/sonic-config-engine/tests/test_cfggen.py", line 836, in test_minigraph_voq_system_ports
2022-05-17T04:17:53.5240263Z     json.loads(self.run_script(argument)),
2022-05-17T04:17:53.5241540Z   File "/sonic/src/sonic-config-engine/tests/test_cfggen.py", line 54, in run_script
2022-05-17T04:17:53.5242388Z     self.assertTrue(self.yang.validate(argument))
2022-05-17T04:17:53.5243050Z AssertionError: False is not true
2022-05-17T04:17:53.5243783Z 

Check the build failure in #10843 for details.
Thanks

@qiluo-msft
Copy link
Collaborator

@ganglyu is working on reverting on 202111.

liushilongbuaa pushed a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Jun 20, 2022
…anch

Related work items: #52, #71, #73, #75, #77, sonic-net#1306, sonic-net#1588, sonic-net#1991, sonic-net#2031, sonic-net#2040, sonic-net#2053, sonic-net#2066, sonic-net#2069, sonic-net#2087, sonic-net#2107, sonic-net#2110, sonic-net#2112, sonic-net#2113, sonic-net#2117, sonic-net#2124, sonic-net#2125, sonic-net#2126, sonic-net#2128, sonic-net#2130, sonic-net#2131, sonic-net#2132, sonic-net#2133, sonic-net#2134, sonic-net#2135, sonic-net#2136, sonic-net#2137, sonic-net#2138, sonic-net#2139, sonic-net#2140, sonic-net#2143, sonic-net#2158, sonic-net#2161, sonic-net#2233, sonic-net#2243, sonic-net#2250, sonic-net#2254, sonic-net#2260, sonic-net#2261, sonic-net#2267, sonic-net#2278, sonic-net#2282, sonic-net#2285, sonic-net#2288, sonic-net#2289, sonic-net#2292, sonic-net#2294, sonic-net#8887, sonic-net#9279, sonic-net#9390, sonic-net#9511, sonic-net#9700, sonic-net#10025, sonic-net#10322, sonic-net#10479, sonic-net#10484, sonic-net#10493, sonic-net#10500, sonic-net#10580, sonic-net#10595, sonic-net#10628, sonic-net#10634, sonic-net#10635, sonic-net#10644, sonic-net#10670, sonic-net#10691, sonic-net#10716, sonic-net#10731, sonic-net#10750, sonic-net#10751, sonic-net#10752, sonic-net#10761, sonic-net#10769, sonic-net#10775, sonic-net#10776, sonic-net#10779, sonic-net#10786, sonic-net#10792, sonic-net#10793, sonic-net#10800, sonic-net#10806, sonic-net#10826, sonic-net#10839, sonic-net#10840, sonic-net#10842, sonic-net#10844, sonic-net#10847, sonic-net#10849, sonic-net#10852, sonic-net#10865, sonic-net#10872, sonic-net#10877, sonic-net#10886, sonic-net#10889, sonic-net#10903, sonic-net#10904, sonic-net#10905, sonic-net#10913, sonic-net#10914, sonic-net#10916, sonic-net#10919, sonic-net#10925, sonic-net#10926, sonic-net#10929, sonic-net#10933, sonic-net#10934, sonic-net#10937, sonic-net#10941, sonic-net#10947, sonic-net#10952, sonic-net#10953, sonic-net#10957, sonic-net#10959, sonic-net#10971, sonic-net#10972, sonic-net#10980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Included in 202111 Branch Request for 202111 Branch For PRs being requested for 202111 branch sonic-cfggen SONiC Configuration Generator Tool YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants