Added detailed reason for assert failure for bgp,sonic,sfp#18777

Merged

StormLiangMS merged 8 commits intosonic-net:masterfrom

bachalla:assert_add

Jun 21, 2025

Contributor

bachalla commented Jun 3, 2025

Description of PR

Added detailed Reason for Assert failure

Type of change

Back port request

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.


          Added_assert_fail_reason

5c1ad56

bachalla requested review from StormLiangMS and prgeor as code owners

June 3, 2025 15:51

bachalla requested a review from ZhaohuiS

June 3, 2025 15:51

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                              "- Output: {}\n"
+                          ).format(
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json

Contributor

ZhaohuiS Jun 4, 2025

Not sure how many keys in routes_json, but for this assert, it's better to print out routes_json['totalPrefixCounter']

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                              "- Output: {}\n"
+                          ).format(
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json

Contributor

ZhaohuiS Jun 4, 2025

for this assert, it's better to print out routes_json['totalPrefixCounter']

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                              "- Output: {}\n"
+                          ).format(
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json

Contributor

ZhaohuiS Jun 4, 2025

for this assert, it's better to print out routes_json['totalPrefixCounter']

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                          ).format(
+                              routes_json['totalPrefixCounter'],
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json

Contributor

ZhaohuiS Jun 4, 2025

Don't print routes_json out, just printing out routes_json['totalPrefixCounter'] is enough

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                              "- Output: {}\n"
+                          ).format(
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json

Contributor

ZhaohuiS Jun 4, 2025

Please just print out routes_json['totalPrefixCounter']

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py Outdated

+                                 "Expected: {expected}, Actual: {actual}. "
+                                 "- Command: {cmd}\n"
+                              ).format(
+                                expected=original_ipv4_route_adv_filter_count,

Contributor

ZhaohuiS Jun 4, 2025

why do you use another format to print the variables, please keep the format same consistently.

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_azng_migration.py

+                          ).format(
+                              bgp_nbr_recv_cmd if 'bgp_nbr_recv_cmd' in locals() else 'N/A',
+                              routes_json
+                          )

Contributor

ZhaohuiS Jun 4, 2025

Pls print out routes_json['totalPrefixCounter']

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_suppress_fib.py Outdated

+                      r.status_code,
+                      url,
+                      data,
+                      r.text

Contributor

ZhaohuiS Jun 4, 2025

Does r have text? If you are not sure, please remove it, otherwise it will cause exception and fail the case

ZhaohuiS reviewed

View reviewed changes

tests/bgp/test_bgp_suppress_fib.py

+                                  "Check the process status with 'ps -ef | grep orchagent', review DUT logs, "
+                                  "and ensure no supervisor is restarting the process."
+                              ).format(duthost.hostname)

Contributor

ZhaohuiS Jun 4, 2025

For those ones have failed reason, I don't suggest to change it. That's the author intention.

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py

                           disable_dom_result = self.duthost.command(self.cmd_disable_dom)
-                          assert disable_dom_result["rc"] == 0, \
-                              "Disable DOM polling failed for {}".format(self.logical_intf)
+                          assert disable_dom_result["rc"] == 0, (

Contributor

ZhaohuiS Jun 4, 2025

For those ones have failed reason, I don't suggest to change it. That's the author intention.

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py

                       shutdown_result = self.duthost.command(self.cmd_down)
-                      assert shutdown_result["rc"] == 0, "Shutdown {} failed".format(self.logical_intf)
-                      assert check_interface_status(self.duthost, [self.logical_intf], expect_up=False)
+                      assert shutdown_result["rc"] == 0, (

Contributor

ZhaohuiS Jun 4, 2025

For those ones have failed reason, I don't suggest to change it. That's the author intention.

bachalla added 3 commits

June 5, 2025 09:23


          Added_assert_fail_reason

07c078b


          Fixed_review_comments

79a5af5


          Merge branch 'sonic-net:master' into assert_add

d00a076

Collaborator

mssonicbld commented Jun 5, 2025

/azp run

azure-pipelines bot commented Jun 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).


          Fixed_review_comments

2bd25f1

Collaborator

mssonicbld commented Jun 5, 2025

/azp run

azure-pipelines bot commented Jun 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).


          Fixed_review_comments

51756d9

Collaborator

mssonicbld commented Jun 5, 2025

/azp run

azure-pipelines bot commented Jun 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py Outdated

-                  assert sfp_presence['rc'] == 0, "Run command '{}' failed".format(cmd_sfp_presence)
+                  assert sfp_presence['rc'] == 0, (
+                      "Run command '{}' failed with return code {}. "
+                      "This means the 'sfputil show presence' command did not execute successfully on the DUT. "

Contributor

ZhaohuiS Jun 6, 2025 •

edited

Loading

Please remove these 2 indication lines. Just print out the reason is enough, don't need to explain or tell the user how to do debug it. "This means.." is not necessary at all

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py Outdated

Comment on lines +376 to +379

+                              "This means the SFP module is not detected as present on this interface. "
+                              "Please verify the SFP module is properly inserted and operational, check compatibility, "
+                              "run 'sfputil show presence' manually, and review DUT logs for related errors."
+                          ).format(intf)

Contributor

ZhaohuiS Jun 6, 2025 •

edited

Loading

Please remove these 3 indication lines. Just print out the reason is enough, don't need to explain or tell the user how to do debug it. "This means.." is not necessary at all

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py Outdated

+                  assert sfp_eeprom['rc'] == 0, (
+                      "Run command '{}' failed with return code {}. "
+                      "This means the 'sfputil show eeprom' command did not execute successfully on the DUT. "
+                      "Please check the command output, DUT logs, and platform status for troubleshooting."

Contributor

ZhaohuiS Jun 6, 2025

"This means.." is not necessary at all, please remove them.

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/sfp/test_sfputil.py Outdated

+                      expect_up=True
+                  ), (
+                      "Not all interfaces that are admin up transitioned to the 'up' state after SFP reset. "
+                      "This means one or more interfaces expected to be operationally up are still down. "

Contributor

ZhaohuiS Jun 6, 2025

"This means.." is not necessary at all. Just tell the failed reason, no need to tell user how to debug or fix it. Make the summary clean and clear.

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/test_reload_config.py Outdated

-                      assert wait_until(300, 10, 0, check_docker_status, duthost)
+                      assert wait_until(300, 10, 0, check_docker_status, duthost), (
+                          "Not all Docker containers reached the 'fully started' state within 300 seconds after config reload."
+                      ).format()

Contributor

ZhaohuiS Jun 9, 2025

same as above

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/test_reload_config.py Outdated

+                  assert wait_until(360, 20, 0, config_system_checks_passed, duthost, delayed_services), (
+                      "System checks did not pass within the allotted time after config reload."
+                  ).format()

Contributor

ZhaohuiS Jun 9, 2025

Please remove format if there is no variable, or add some variables in format.

ZhaohuiS reviewed

View reviewed changes

tests/platform_tests/test_reload_config.py Outdated

+                  assert wait_until(360, 20, 0, config_system_checks_passed, duthost, delayed_services), (
+                      "System checks did not pass within the allotted time after config reload."
+                  ).format()

Contributor

ZhaohuiS Jun 9, 2025

Please remove format if there is no variable, or add some variables in format.


          Fixed_review_comments

16c0795

Collaborator

mssonicbld commented Jun 9, 2025

/azp run

azure-pipelines bot commented Jun 9, 2025

Azure Pipelines successfully started running 1 pipeline(s).

ZhaohuiS approved these changes

View reviewed changes

Contributor

ZhaohuiS left a comment

Looks good to me now.

Contributor

ZhaohuiS commented Jun 9, 2025

@StormLiangMS Please help review. Thanks.

Contributor

ZhaohuiS commented Jun 16, 2025

@bachalla Please add abstracted scripts name in the title.

bachalla changed the title ~~Added detailed reason for assert failure~~ Added detailed reason for assert failure for bgp,sonic,sfp

StormLiangMS approved these changes

View reviewed changes

Collaborator

StormLiangMS left a comment

hi @bachalla @ZhaohuiS I think this enhancement is great and will be very helpful for issue triage. Would you mind creating a document that categorizes the enhancements made for each case? I'd like to include this as part of the best practices in the sonic-mgmt codebase.

StormLiangMS merged commit 903edbe into sonic-net:master

18 checks passed

StormLiangMS added the Request for 202505 branch label

yejianquan added the Approved for 202505 branch label

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

45c051c

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

mssonicbld added the Created PR to 202505 branch label

Collaborator

mssonicbld commented Jun 21, 2025

Cherry-pick PR to 202505: #19125

mssonicbld mentioned this pull request

[action] [PR:18777] Added detailed reason for assert failure for bgp,sonic,sfp #19125

Merged

11 tasks

mssonicbld pushed a commit that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (#18777)

e917fea

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

mssonicbld added Included in 202505 branch and removed Created PR to 202505 branch labels

nissampa pushed a commit to nissampa/sonic-mgmt_dpu_test that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

0fbe8a4

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

opcoder0 pushed a commit to opcoder0/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

9c43d36

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>

gshemesh2 pushed a commit to gshemesh2/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

911316b

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: Guy Shemesh <gshemesh@nvidia.com>

AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

de8275f

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: Aharon Malkin <amalkin@nvidia.com>

gshemesh2 pushed a commit to gshemesh2/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

03df90f

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: Guy Shemesh <gshemesh@nvidia.com>

venu-nexthop pushed a commit to venu-nexthop/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

3eed0ac

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

gshemesh2 pushed a commit to gshemesh2/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

18f36d9

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: Guy Shemesh <gshemesh@nvidia.com>

ytzur1 pushed a commit to ytzur1/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

5e0f93d

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Signed-off-by: Yael Tzur <ytzur@nvidia.com>

venu-nexthop pushed a commit to venu-nexthop/sonic-mgmt that referenced this pull request


          Added detailed reason for assert failure for bgp,sonic,sfp (sonic-net…

9dd720d

…#18777)

Summary:

Enhanced assertion messages in selected test cases to provide clearer failure context. This improves debuggability and speeds up issue resolution when tests fail.
Added detailed assertion failure messages in test scripts to make it easier to understand why tests fails.

What is the motivation for this PR?
The motivation is to improve test debuggability by adding detailed failure reasons in assertions for selected test cases. This enhancement makes it easier to identify the root cause of test failures in logs, thereby reducing triage time and simplifying test maintenance.

How did you do it?
I updated the assertion statements in the following test files to include descriptive error messages:
tests/bgp/test_bgp_azng_migration.py
tests/bgp/test_bgp_suppress_fib.py
tests/common/devices/sonic.py
tests/platform_tests/sfp/test_sfputil.py
tests/platform_tests/sfp/test_show_intf_xcvr.py
tests/platform_tests/test_reload_config.py

How did you verify/test it?
Verified that the updated assertion messages are correctly reflected in the test code by reviewing the changes locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved for 202505 branch Included in 202505 branch Request for 202505 branch