Test Completeness enhancements - new diagnose level and helper methods by vaibhavhd · Pull Request #2149 · sonic-net/sonic-mgmt

vaibhavhd · 2020-08-27T21:29:19Z

Description of PR

Summary: Enhancements to test-completeness-infra
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

Changes:

New level - diagnose - If this level is specified and a test doesn’t support it, then it shall fall back to basic mode. This level doesn’t have any implication of above or below any existing level. This level is not the same as debug level, which is intended to take a sample test to debug test code. diagnose level is to diagnose sonic feature/behavior.
New completeness handler for getting the normalized level from the request attribute.
New completeness handler to get level name in str type from the type CompletenessLevel.
Modified usage of completeness level in test_link_flap testcase.

How did you verify/test it?

Tested test_link_flap testcase successfully.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

daall

LGTM, just a few clarification points.

daall · 2020-08-31T16:09:35Z

tests/common/plugins/test_completeness/README.md

-        Thorough
+        thorough
+
+An unordered level `diagnose` is also supported. If `diagnose` level is specified and a test doesn’t support it, then it shall fall back to `basic` mode. This level doesn’t have any implication of above or below any existing level. Furthermore, this level is not the same as `debug` level, which is intended to take a sample test to debug test code. `diagnose` level is to be used to diagnose sonic feature/behavior.


diagnose level is used to diagnose sonic feature/behavior.

Can you clarify this some more? I think it is still a little confusing what the difference is between this new level and the existing levels. Maybe an example use-case would help?

Improved the description and described what the level is ideally meant for.

daall · 2020-08-31T16:11:28Z

tests/platform_tests/test_link_flap.py

-    tlf = TestLinkFlap()
-    tlf.run_link_flap_test(duthost, fanouthosts, normalized_level)
+    tlf = TestLinkFlap(request)
+    tlf.run_link_flap_test(duthost, fanouthosts)


Missing newline at the end. (I know you didn't add this but since we're here. 😄)

daall · 2020-08-31T16:14:38Z

tests/common/plugins/test_completeness/__init__.py

+        """Takes a request instance and returns normalized completeness level in string format.
+        For example, if a testcase supports "CompletenessLevel.basic, CompletenessLevel.thorough", and specified level
+        during test execution is "confident", this method will make use of normalization logic during
+        pytest_runtest_setup and returns the normalized level as "basic" (str type of CompletenessLevel.basic)
+        """


I have a few suggestions here:

Separate the summary line and the example

Generalize the description a little bit (i.e. describe what is happening rather than how it is happening)

Explicitly call out the return type

One possible interpretation:

Suggested change

"""Takes a request instance and returns normalized completeness level in string format.

For example, if a testcase supports "CompletenessLevel.basic, CompletenessLevel.thorough", and specified level

during test execution is "confident", this method will make use of normalization logic during

pytest_runtest_setup and returns the normalized level as "basic" (str type of CompletenessLevel.basic)

"""

"""Get the normalized completeness level for a given test instance.

For example, if a testcase supports "CompletenessLevel.basic, CompletenessLevel.thorough", and the specified level

during test execution is "confident", then this method will normalize the level to "basic".

Returns:

CompletenessLevel as a string

"""

Incorporated the suggestion. Thanks!

daall · 2020-08-31T16:20:20Z

tests/common/plugins/test_completeness/__init__.py

+        """Converts a type CompletenessLevel to type str.
+        For example, if input is CompletenessLevel.basic, this method will return "basic"
+        Arguments:
+        level - An enum value of type CompletenessLevel


Just some minor formatting changes:

Suggested change

"""Converts a type CompletenessLevel to type str.

For example, if input is CompletenessLevel.basic, this method will return "basic"

Arguments:

level - An enum value of type CompletenessLevel

"""Converts a type CompletenessLevel to type str.

For example, if input is CompletenessLevel.basic, this method will return "basic."

Arguments:

level - An enum value of type CompletenessLevel

Incorporated the suggestion. Thanks!

neethajohn · 2020-09-01T21:26:55Z

tests/common/plugins/test_completeness/__init__.py

-    basic = 1
-    confident = 2
-    thorough = 3 # Maximum execution
+    diagnose = 0


you did mention this is an unordered level. do you need to shift the other levels in that case? Wouldn't specifying some value for 'diagnose' level suffice?

Any int value can be assigned to diagnose. It has to be int due to base type enum.IntEnum.
During normalization, there is a special handling for diagnose which does not consider ordering.

I started with enum.IntEnum as the levels were assumed to follow a ordered priority. But this does not seem to be the case anymore. Removing base class inheritance will make this list look simple, but then the downside is we will have to implement methods to handle ordered levels, exists check, etc. These methods are presently used from enum.IntEnum

Just to keep the change minimal, you can assign diagnose some int value since its treated differently

Do you mean any int value other than 0?

yes. some large value leaving enough space after max level in case we need to add more ordered levels in future

vaibhavhd · 2020-09-02T22:17:59Z

retest vsimage please

988d8e172e7140174bfa21d3b86c9685c7127a14 (HEAD -> 201911, origin/201911) [warm-reboot]: added automated recover for ISSU file (sonic-net#1466) 913df4e2faf6e70e0aebb01d81f79694a6d8ee20 [201911] Warmboot script improvements - timeout in exec and disable service-autorestart (sonic-net#2149) Signed-off-by: Abhishek Dosi <[email protected]>

Test Completeness enhancements - diagnose level, helper methods

dedc179

vaibhavhd added Enhancement Pytest 🐍 labels Aug 27, 2020

vaibhavhd self-assigned this Aug 27, 2020

vaibhavhd requested a review from a team August 27, 2020 21:29

Description added for new level diagnose

e64bf79

daall reviewed Aug 31, 2020

View reviewed changes

Improve test completeness documentation

7bbd373

vaibhavhd requested a review from daall September 1, 2020 18:20

neethajohn reviewed Sep 1, 2020

View reviewed changes

Diagnose level numbering modified to a large value

9705fc6

vaibhavhd requested a review from neethajohn September 2, 2020 18:58

Revert the ordering for completeness levels

4fde170

neethajohn approved these changes Sep 2, 2020

View reviewed changes

vaibhavhd merged commit 96466d5 into sonic-net:master Sep 3, 2020

vaibhavhd deleted the completeness-improvements branch September 3, 2020 06:30

-        """Takes a request instance and returns normalized completeness level in string format.
-        For example, if a testcase supports "CompletenessLevel.basic, CompletenessLevel.thorough", and specified level
-        during test execution is "confident", this method will make use of normalization logic during
-        pytest_runtest_setup and returns the normalized level as "basic" (str type of CompletenessLevel.basic)
-        """
+        """Get the normalized completeness level for a given test instance.
+        For example, if a testcase supports "CompletenessLevel.basic, CompletenessLevel.thorough", and the specified level
+        during test execution is "confident", then this method will normalize the level to "basic".
+         Returns:
+             CompletenessLevel as a string
+        """

Conversation

vaibhavhd commented Aug 27, 2020

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

daall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaibhavhd Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaibhavhd commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vaibhavhd Sep 1, 2020 •

edited

Loading