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

Add unit test for conditional marks. #17005

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yutongzhang-microsoft
Copy link
Contributor

@yutongzhang-microsoft yutongzhang-microsoft commented Feb 18, 2025

Description of PR

This PR is based on PR #16930. After changing the logic of conditional marks, we need to add unit test to make sure it works as expected.

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405
  • 202411

Approach

What is the motivation for this PR?

This PR is based on PR #16930. After changing the logic of conditional marks, we need to add unit test to make sure it works as expected.

How did you do it?

How did you verify/test it?

yutongzhang@sonic_mgmt:/data/sonic-mgmt$ python -m pytest --noconftest --capture=no tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py -v -s

tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_1 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_2 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_all_false_conditions_in_matching_path_3 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_contradicting_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_defalut_logic_operation PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_duplicated_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_empty_conditions PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_logic_operation_and PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_logic_operation_or PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_no_matches PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_partly_false_conditions_in_longest_entry PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_true_conditions_in_longest_entry PASSED

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin
Copy link
Collaborator

wangxin commented Feb 19, 2025

For unitest, please do not use file name with pattern "test_*.py". Pytest considers all file names with such pattern as test scripts. They could be collected by pytest as normal test scripts.
Pytest also support some other file name patterns. Please check the pytest doc and use a non-conflicting file name pattern for unit test scripts.

Copy link
Contributor

@congh-nvidia congh-nvidia left a comment

Choose a reason for hiding this comment

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

LGTM

@yutongzhang-microsoft
Copy link
Contributor Author

yutongzhang-microsoft commented Feb 19, 2025

For unitest, please do not use file name with pattern "test_*.py". Pytest considers all file names with such pattern as test scripts. They could be collected by pytest as normal test scripts. Pytest also support some other file name patterns. Please check the pytest doc and use a non-conflicting file name pattern for unit test scripts.

Hi, @wangxin , I changed to use unittest framework here, which pytest can support. The script will be named as unittest_*.py.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin
Copy link
Collaborator

wangxin commented Feb 19, 2025

Other things need to consider:

  • Add document.
  • Add more samples to cover more cases, especially corner/boundary cases

@yutongzhang-microsoft
Copy link
Contributor Author

Other things need to consider:

  • Add document.
  • Add more samples to cover more cases, especially corner/boundary cases

The document for unittest or for explaining the logic for conditional mark? If the second one, can I update it in PR #16930, as this PR is for unittest. Maybe I can also add the explaination for the unittest in this PR.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin
Copy link
Collaborator

wangxin commented Feb 24, 2025

Other things need to consider:

  • Add document.
  • Add more samples to cover more cases, especially corner/boundary cases

The document for unittest or for explaining the logic for conditional mark? If the second one, can I update it in PR #16930, as this PR is for unittest. Maybe I can also add the explaination for the unittest in this PR.

Add document for how to run unit test.

@wangxin
Copy link
Collaborator

wangxin commented Feb 24, 2025

For unit test, it is supposed to cover boundary and corner cases. There are many more scenarios to be covered. For example:

  1. Empty condition.
  2. Logic operation between conditions.
  3. Duplicated conditions.
  4. Contradicting conditions (same level, different level)
  5. etc.

@yutongzhang-microsoft
Copy link
Contributor Author

Other things need to consider:

  • Add document.
  • Add more samples to cover more cases, especially corner/boundary cases

The document for unittest or for explaining the logic for conditional mark? If the second one, can I update it in PR #16930, as this PR is for unittest. Maybe I can also add the explaination for the unittest in this PR.

Add document for how to run unit test.

Design document for unittest is done.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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