Skip to content

Tests mark constants#17707

Open
rbpittman wants to merge 8 commits intosonic-net:masterfrom
rbpittman:tests_mark_constants
Open

Tests mark constants#17707
rbpittman wants to merge 8 commits intosonic-net:masterfrom
rbpittman:tests_mark_constants

Conversation

@rbpittman
Copy link
Contributor

@rbpittman rbpittman commented Mar 26, 2025

Description of PR

Summary:
Enables support for constant declaration in a separate yaml file.
Allows high-level conditions to be specified once with much less duplication. Failure to detect duplication can be hazardous and can result in skips that are never detected unless every test is manually audited.
Reduced the duplicated QOS sai topo listing to use a single constant. Every instance was identical.

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?

How did you do it?

How did you verify/test it?

Added to the tests in the unittest file and executed tests against hash 8058c87.
Workspace:

sonic-mgmt$ git log -n 1                                                       
commit 8058c87589d88214551d8cf22fd875f5684b79bb (HEAD -> tests_mark_constants, origin/tests_mark_constants)     
Author: ...                                                           
Date:   Mon Mar 24 18:31:13 2025 +0000                                                                          
                                                                                                                
    Remove excess yaml code.               

Test results:

data$ python -m pytest --noconftest --capture=no tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py -v -s
ERROR:pytest_ansible.units:No galaxy.yml file found, plugin not activated
=============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.8.10, pytest-7.4.0, pluggy-1.5.0 -- /usr/bin/python
cachedir: .pytest_cache
metadata: {'Python': '3.8.10', 'Platform': 'Linux-5.15.0-131-generic-x86_64-with-glibc2.29', 'Packages': {'pytest': '7.4.0', 'pluggy': '1.5.0'}, 'Plugins': {'forked': '1.6.0', 'xdist': '1.28.0', 'metadata': '3.1
.1', 'repeat': '0.9.3', 'html': '4.1.1', 'allure-pytest': '2.8.22', 'ansible': '4.0.0'}}
ansible: 2.13.13
rootdir: /data/tests
configfile: pytest.ini
plugins: forked-1.6.0, xdist-1.28.0, metadata-3.1.1, repeat-0.9.3, html-4.1.1, allure-pytest-2.8.22, ansible-4.0.0
collected 21 items

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_constants_boolean PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_constants_chain PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_constants_negative 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_only_use_the_longest_1 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_2 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_3 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_4 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_5 PASSED
tests/common/plugins/conditional_mark/unit_test/unittest_find_all_matches.py::TestFindAllMatches::test_only_use_the_longest_6 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

================================================================================================ warnings summary =================================================================================================
../usr/local/lib/python3.8/dist-packages/paramiko/transport.py:236
  /usr/local/lib/python3.8/dist-packages/paramiko/transport.py:236: CryptographyDeprecationWarning: Blowfish has been deprecated
    "class": algorithms.Blowfish,

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================================================================== 21 passed, 1 warning in 0.77s ==========================================================================================
data$

Secondarily validated that test_qos_sai.py execution list looks correct on 8101 when using hash 06444f8 on this branch.

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@XuChen-MSFT
Copy link
Contributor

@yutongzhang-microsoft
can you take a look at this PR? to support for constant in condition mark file

@yutongzhang-microsoft
Copy link
Contributor

Hi, @rbpittman , thank you for your contribution. But actually, we don't think it is necessary to do this. As in #16930, we have refined the logic of conditional mark again, and after the refine, we don't have to copy-paste the same condition, only leave it in the shorter matching is ok.

@rbpittman
Copy link
Contributor Author

@yutongzhang-microsoft
That may not be necessary for QOS SAI in particular, but there's several other locations where different test marks for unrelated files have the same underlying cause, for which a constant could be used to ensure each instance of a mark doesn't drift apart with future commits.
For example, there are several repeated uses of:

  • "topo_type in ['m0', 'mx', 'm1', 'm2', 'm3']"
  • ['td2', 'spc1', 'spc2', 'spc3', 'spc4']
  • platform.startswith('x86_64-8122_')

These reflect underlying asic classifications that could be commonized to ensure updates to the value are not missed out, and they take place across very differently named test cases.

Separately, is there a plan to remove the QOS SAI topo duplicated list?

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