Skip to content

Making norun_tests env_types combinations robust #205

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.

Sign up for GitHub

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

Open
narasimhan-v wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Making norun_tests env_types combinations robust #205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions avocado-setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

from lib.logger import logger_init
from lib import helper
from itertools import permutations

BASE_PATH = os.path.dirname(os.path.abspath(__file__))
CONFIG_PATH = "%s/config/wrapper/env.conf" % BASE_PATH
Expand Down Expand Up @@ -443,14 +444,17 @@ def parse_test_config(test_config_file, avocado_bin, enable_kvm):
(env_ver, env_type, cmdpat) = helper.get_env_type(enable_kvm)
norun_tests = []
# Get common set of not needed tests
env = 'norun_%s' % env_type
dist = 'norun_%s' % helper.get_dist()[0]
major = 'norun_%s' % env_ver.split('.')[0]
minor = 'norun_%s' % env_ver
minor_env = 'norun_%s_%s' % (env_ver, env_type)
for section in [env, dist, major, minor, minor_env]:
if NORUNTESTFILE.has_section(section):
norun_tests.extend(NORUNTESTFILE.get(section, 'tests').split(','))
dist = helper.get_dist()[0]
major = env_ver.split('.')[0]
minor = env_ver
perm = []
env_list = [env_type, dist, major, minor]
for length in range(1, len(env_list) + 1):
Copy link
Collaborator

@harish-24 harish-24 Apr 22, 2020

Choose a reason for hiding this comment

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

Considering we have all combinations covered at length 2, do we need a length of 3 and 4? Just a thought that we can cover what we need at length 1 and 2.

norun_NV
norun_rhel
norun_rhel8
norun_rhel8.1
norun_NV_rhel
norun_NV_rhel8
norun_NV_rhel8.1
norun_rhel_NV
norun_rhel_rhel8
norun_rhel_rhel8.1
norun_rhel8_NV
norun_rhel8_rhel
norun_rhel8_rhel8.1
norun_rhel8.1_NV
norun_rhel8.1_rhel
norun_rhel8.1_rhel8

It also matches what we have in the config now. Config would be better to maintain this way.
Possibly we can extend the length when needed, but I doubt if we would need to.

What are your thoughts here?

Copy link
Collaborator Author

@narasimhan-v narasimhan-v Apr 22, 2020

Choose a reason for hiding this comment

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

Possibly we can extend the length when needed, but I doubt if we would need to.

You are right in this case alone. Because we have rhel, rhel8, rhel8.1 as different entries, we have this luxury to cover all needed combinations. And since we are not doing too much extra work to generate those extra permutations, I have included all possible ones.

It also matches what we have in the config now. Config would be better to maintain this way.

We are not going to touch the config. Just that the current config limits users in certain ways (like not able to cover certain permutations), which this code the future config can have more entries, but this in no way forces changes or limitations in current config.

Copy link
Collaborator

@harish-24 harish-24 Apr 22, 2020

Choose a reason for hiding this comment

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

We are not going to touch the config. Just that the current config limits users in certain ways (like not able to cover certain permutations), which this code the future config can have more entries, but this in no way forces changes or limitations in current config.

My only worry was not to make the config clumsy. Because when we have various ways to use the config we may tend to use different section name for same type of use case.

Ex.

  1. norun_rhel8.1_NV
  2. norun_rhel_rhel8_rhel8.2_NV

But yes of course it does not harm anything. So it is your call if we can go with current implementation.

Copy link
Collaborator Author

@narasimhan-v narasimhan-v Apr 22, 2020

Choose a reason for hiding this comment

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

I get your point. My goal was to make it easier for user to go with any order they want, without knowing what order the code computes (this should ideally be the case as well).

  1. Does it make the config file clumsy in future - Yes
    2a. Does it cover all possible permutations - Yes
    2b. Does it make the user's job easier to create whatever permutation they want to - Yes

For me, advantages of 2a and 2b weigh in more than the clumsiness, and the disadvantages of that clumsiness is something I don't mind :)

On a side note, I was even planning to remove the norun_ from the config section names, since that is irrelevant.

Copy link
Collaborator

@sathnaga sathnaga Apr 22, 2020

Choose a reason for hiding this comment

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

@narasimhan-v one of the concern that I had is already getting discussed here, I agree with @harish-24, we do not need more depth combinations, rather let's try to have layered, but that is not possible with current implementation or topic for current one, let's have another discussion[1].

I would say it is better not to confuse user, and we need to have some rules for user so the documentation, I do NOT agree on what ever user give we should should parse, probably we can add the example for valid patterns that user can provide.

    Some tests have the potential of causing system crashes / hangs. Such test in cfg file hinders the run of tests that follow.
    So, if such tests are identified and need to be "not run" for a particular environment (Since they can run fine on others),
    we have a provision to mention such tests to not run.
    Please have a look at "config/wrapper/no_run_tests.conf"

how about make it generic, I guess it is already we use it but not in the order, probably that can be corrected, which would be cleaner addition for user, what say?

Platform:
Platform_Distro:
Platform_Distro_MajorNo:
Platform_Distro_MajorNo_MinorNo:

example for each :
Platform: KVM, NV, PHYP
Distro: rhel, sles, ubuntu
MajorNo: 8, 11, 14
MinorNo: 2, sp4, 04

Additionally, what is the issue with current implementation and why we need this change is not clear from commit message, for me it looks like we are trying to ease user for his wrongly configured environment, which might lead other failures going forward.

[1]: How about having a yaml implementation of norun config, so that we can approach it as
layered, am not big user/fan of yaml, I know you have been using in avocado misc tests, and i feel it would suite this need for long run and we can avoid duplication, this can wait and we can discuss more on such design.

Copy link
Collaborator Author

@narasimhan-v narasimhan-v Apr 22, 2020

Choose a reason for hiding this comment

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

Additionally, what is the issue with current implementation and why we need this change is not clear from commit message.

To start with, we are not allowing users to provide norun tests for these type of combinations now.

norun_phyp
norun_rhel_phyp

Another issue we will face is, we are not at all looking at processor generation and system type today (P8 vs P9, OpenPower vs IBM), and these are valid use cases where some people do not want a test to run on certain systems where it is harmful.
Forcing users to go with a particular order is not intuitive. I, as a user, will feel norun_P9_NV_... is right, and someone else might feel norun_NV_P9 is right. And we should not be spending time on defining the right order, IMO.

How about having a yaml implementation of norun config

I like this idea for all the reasons above, and will explore more on this.

Copy link
Collaborator

@sathnaga sathnaga Apr 22, 2020

Choose a reason for hiding this comment

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

Additionally, what is the issue with current implementation and why we need this change is not clear from commit message.

To start with, we are not allowing users to provide norun tests for these type of combinations now.

norun_phyp
norun_rhel_phyp

Another issue we will face is, we are not at all looking at processor generation and system type today (P8 vs P9, OpenPower vs IBM), and these are valid use cases where some people do not want a test to run on certain systems where it is harmful.
Forcing users to go with a particular order is not intuitive. I, as a user, will feel norun_P9_NV_... is right, and someone else might feel norun_NV_P9 is right. And we should not be spending time on defining the right order, IMO.

Right order helps to avoid confusion, moreover I feel, there is no liking of user needed here, this is to get some job done for user, and rules are always good to keep the coding/processing sane.
More over it will help user identifying if the pattern is already defined, I feel that is more of a usability we need to provide user over any pattern.

Good, we need Processor Generation too, that's where defined pattern would help us.

Small background on KVM test suite: I had a similar need on KVM tests and that resulted in almost 6 months refactoring cpu utils code on avocado but it is cleaner now, the actual change I wanted to make it is still in-review, there we have a Cartesian config which came handy and test implementation there would support such change, similar approach would be good conceptually, I believe yaml will help us.

https://github.com/avocado-framework/avocado-vt/pull/2225
https://github.com/autotest/tp-libvirt/pull/2301

How about having a yaml implementation of norun config

I like this idea for all the reasons above, and will explore more on this.

Good, pls do, this might ease lot of things we discuss already.
My suggestion would be to park this PR for any rework, and explore on yaml way of doing....

Copy link
Collaborator Author

@narasimhan-v narasimhan-v Apr 22, 2020

Choose a reason for hiding this comment

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

Sure

perm.extend(list(permutations(env_list, length)))
perm = ['norun_%s' % '_'.join(per) for per in perm]
for per in perm:
if NORUNTESTFILE.has_section(per):
norun_tests.extend(NORUNTESTFILE.get(per, 'tests').split(','))
norun_tests = list(filter(None, norun_tests))

with open(test_config_file, 'r') as fp:
Expand Down