[conditional marks]: the matching rule of conditional marks take only one conditional, for same mark from different files

[AntonHryshchuk]

Issue Description

Introduced in #14395,
After the change, if a similar mark exists in different files, only the last (alphabet order) will be taken.
In same length of case.

A.yaml:

package/test.py:
   skip:
      condition:
         - conditionA

B.yaml:

package/test.py:
   skip:
      condition:
         - conditionB

previously, the method find_longest_matches returned:
[{'packge/test.py':{'skip':{'condition':["conditionA"]}}}, {'packge/test.py':{'skip':{'condition':["conditionB"]}}}]

Now the return value of the method find_all_matches will be
[{'packge/test.py':{'skip':{'condition':["conditionB"]}}}]

Results you see

only last condition taken

Results you expected to see

if several files have the same length of case and same mark, all these skip conditions will be taken

Is it platform specific

generic

Relevant log output

No response

Output of show version

No response

Attach files (if any)

No response

[yxieca]

@yutongzhang-microsoft please make an architectural call on this issue. I am leaning towards to agree that a single condition should be placed in one file only. Can we have a unit test to prevent/warn such scenarios?

@AntonHryshchuk can you share the use case regarding why a single test's skip conditions should be put into 2 files?

[yutongzhang-microsoft]

I am leaning towards to agree that a single condition should be placed in one file only

Hi, @yxieca , we are not allow to duplicate conditions exist in a file, but we don't have a unit test to such scenrios.

@AntonHryshchuk , only last condition taken is what we respected, if the mark is same, we will only take the longest one, as all conditions are sorted in alphabet, we will only take the latest one. Can you share which condition is duplicate?

[congh-nvidia]

Hi @yxieca @yutongzhang-microsoft,

Actually the problem we have is in the longest match logic, because we have our internal skip yaml files and there could be exactly the same entry as in the community files.
I my opinion, the longest match is not a very user friendly approach, for example:

sonic-mgmt/tests/common/plugins/conditional_mark/tests_mark_conditions.yaml

Lines 62 to 85 in f1de2fa

	#######################################
	##### bfd #####
	#######################################
	bfd/test_bfd.py:
	skip:
	reason: "Test not supported for platforms other than Nvidia 4600c/4700/5600 and cisco-8102. Skipping the test"
	conditions:
	- "(release in ['201811', '201911']) or (platform not in ['x86_64-mlnx_msn4600c-r0', 'x86_64-mlnx_msn4700-r0', 'x86_64-nvidia_sn5600-r0', 'x86_64-8102_64h_o-r0', 'x86_64-8101_32fh_o-r0'])"
	bfd/test_bfd.py::test_bfd_basic:
	skip:
	reason: "Test not supported for cisco as it doesnt support single hop BFD
	and not supported for platforms other than Nvidia 4600c/4700/5600. Skipping the test"
	conditions_logical_operator: or
	conditions:
	- "platform in ['x86_64-8102_64h_o-r0', 'x86_64-8101_32fh_o-r0', 'x86_64-8111_32eh_o-r0', 'x86_64-8122_64eh_o-r0', 'x86_64-8122_64ehf_o-r0']"
	- "platform not in ['x86_64-mlnx_msn4600c-r0', 'x86_64-mlnx_msn4700-r0', 'x86_64-nvidia_sn5600-r0', 'x86_64-8102_64h_o-r0', 'x86_64-8101_32fh_o-r0']"
	- "release in ['201811', '201911']"
	bfd/test_bfd.py::test_bfd_echo_mode:
	skip:
	reason: "https://github.com/sonic-net/sonic-mgmt/issues/14087"
	conditions:
	- "https://github.com/sonic-net/sonic-mgmt/issues/14087"

In these skips, if we want to skip test case bfd/test_bfd.py::test_bfd_basic, we need also add the conditions of bfd/test_bfd.py.
And for test case bfd/test_bfd.py::test_bfd_echo_mode, the condition is actually not correct, if the issue is closed, test will start to run on the incorrect releases and platforms.

And why do we have this issue now, because actually there was a bug in the longest match logic(it took all conditions actually, not only the longest one) and the PR(#14395) fix it(we only merged this PR to our local repo recently).

So may I propose a new approach instead of longest match? We can collect all the matches, and then sort them by length on the mark(skip, xfail, etc.) basis, and then evaluate the conditions from the longest match to the shortest. Whenever the condition is True, we add the mark and break.
If we do in this way, we can just add the skips like this:

bfd/test_bfd.py: 
   skip: 
     reason: "Test not supported for platforms other than Nvidia 4600c/4700/5600 and cisco-8102. Skipping the test" 
     conditions: 
       -  "(release in ['201811', '201911']) or (platform not in ['x86_64-mlnx_msn4600c-r0', 'x86_64-mlnx_msn4700-r0', 'x86_64-nvidia_sn5600-r0', 'x86_64-8102_64h_o-r0', 'x86_64-8101_32fh_o-r0'])" 
  
 bfd/test_bfd.py::test_bfd_basic: 
   skip: 
     reason: "Test not supported for cisco as it doesnt support single hop BFD 
              and not supported for platforms other than Nvidia 4600c/4700/5600. Skipping the test" 
     conditions: 
       - "platform in ['x86_64-8102_64h_o-r0', 'x86_64-8101_32fh_o-r0', 'x86_64-8111_32eh_o-r0', 'x86_64-8122_64eh_o-r0', 'x86_64-8122_64ehf_o-r0']" 
  
 bfd/test_bfd.py::test_bfd_echo_mode: 
   skip: 
     reason: "https://github.com/sonic-net/sonic-mgmt/issues/14087" 
     conditions: 
       - "https://github.com/sonic-net/sonic-mgmt/issues/14087" 

When running the test case test_bfd_echo_mode, it will firstly evaluate the condition of the issue, and then the release and platform.

If you agree with this approach, we can commit it. Please share your thoughts. Thanks.

[yxieca]

@congh-nvidia can you book a meeting with @yutongzhang-microsoft to have a discussion? Your example should have been covered by existing logic already.

Your requirement of having private condition file is valid. If we figure out a way to specify which file should take higher priority, does that solve your issue too?

[yutongzhang-microsoft]

@congh-nvidia As ying suggested, we can book a meeting to discuss. You can contact me with [email protected]

[yutongzhang-microsoft]

if we want to skip test case bfd/test_bfd.py::test_bfd_basic, we need also add the conditions of bfd/test_bfd.py.

For statement if we want to skip test case bfd/test_bfd.py::test_bfd_basic, we need also add the conditions of bfd/test_bfd.py , it is not correct. If we want to skip test case bfd/test_bfd.py::test_bfd_basic, and the entry bfd/test_bfd.py::test_bfd_basic exist in the yaml file, we only need to add the conditions under this entry.