search

teemtee / tmt

Test Management Tool
MIT License
76 stars 114 forks source link

Add support for the Test Case Relevancy #101

Closed psss closed 3 years ago

psss commented 4 years ago

It would be nice to have the Test Case Relevancy supported and naturally integrated into the tmt run command so that irrelevant tests can be easily filtered out before the execution. Possibly tmt test command could support filtering based on relevancy as well.

The question is how the environment dimensions should be provided/detected. There could be an option which would allow to manually define one or more dimensions of the environment (similarly as workflow-tomorrow does), some of the dimensions could/should be auto-detected, for example distro from the provision step.

Let's discuss the implementation details here. This is blocking test open sourcing so we should look into this as soon as possible.

psss commented 4 years ago

@lukaszachy, @thrix, @hegerj, @bachradsusi, please share your thoughts.

bachradsusi commented 4 years ago

From my POV as a user of this:

I'd like to have at least a distro auto detection so I don't have to update filter command every time a new Fedora branch is released.

And I'd to have an option to manually define distro for testing purposes, e.g. to be able to manually generate lists of tests for different Fedoras

psss commented 4 years ago

Example selinux pull requests:

psss commented 4 years ago

Test case relevancy specification update proposed in #102.

psss commented 4 years ago

@lukaszachy, I guess you would like to take care of this issue?

lukaszachy commented 3 years ago

Here is the proposal for "relevancy" usage in tmt/fmf as discussed yesterday. Please raise objections in this issue. Implementation itself should happen on several places. I'm still calling it relevancy here for the lack of better word

Relevancy consist of two parts a) Common dimension names and values This belongs to TMT and should be autodetect based on Plan, Artifact, Provision and so on Specific definitions are yet to be agreed on but to illustrate options: distro, arch, component, product, fips, collection, purpose

b) Relevancy Language and FMF integration This is main topic of this issue from now on.

Integration into FMF

Is done via extending TMT with new attribute containing relevancy rules. This attribute is of type list, each item is dictionary of minimum two key-value pairs. Name of this attribute can be arbitrary and will be defined in TMT (as opposed to FMF format itself)

Format

Required keys are:

Optional key:

Example:

/test_name:
  duration: 5m
  rule:
     - duration: 1h
       continue: True
       when: distro < rhel-7
     - enabled: False
       when: CONDITION

In summary, attribute names rule, continue and when are reserved for 'relevancy' purposes.

Order of processing

  1. Elasticity of FMF files is processed first. It applies also on rule attribute - you can use inheritance there as well.
  2. On final FMF metadata rule attribute is evaluated based on the dimension values supplied by the caller.
    • Rule execution happens in the order rules where specified
    • Last manipulation with the attribute values wins

Relevancy language

Changes from current implementation

Syntax

CONDITION ::= expression (bool expression)*
expression ::= dimension operator values
bool ::= '&&' | '||'
dimension ::= [[:alnum:]]+
operator ::= '=' | '==' | '!=' | '<' | '<=' | '>' | '>=' | 'contains' | '!contains' | 'defined' | '!defined'
values ::= value (',' value)*
value ::= [[:alnum:]]+

Operators 'defined' and '!defined' do not have a right side.

Conversion from old format

distro < rhel-8: False converts to 

rule:
 - enabled: False
   when: distro < rhel-8

distro < rhel-8: Foo=bar converts to 

rule:
 - environment:
      Foo: bar
   when: distro < rhel-8
   continue: False

Multiple rules converts 1:1 just continue: False has to be set

psss commented 3 years ago

Thanks for putting the spec together, @lukaszachy. Looks good to me. I'm just a little be confused about the continue default value. By default it is False which means that the first matching rule wins, right? So why are you setting it explicitly in the last example?

psss commented 3 years ago

Perhaps two more things from the original specification. Do we want/need to preserve the major/minor mode comparison the same way?

Plus explicitly mention what happens when environment dimension is not defined:

Rules which contain environment parameters which are not known at the time of evaluation will be skipped.

lukaszachy commented 3 years ago

I'm just a little be confused about the continue default > By default it is False which means that the first matching rule wins, right? So why are you setting it explicitly in the last example?

I thought we decided to go with the default being continue: True. Good point that it has to be specified in the documentation. We have some time to decide what default is the best.

lukaszachy commented 3 years ago

Do we want/need to preserve the major/minor mode comparison the same way?

It matters only for RHEL/CentOS (there are no minor releases of Fedora). And IMHO we have to keep it - e.g. test is relevant from rhel-6.7, 7.3 and 8.2 (example situation, e.g. new feature or CVE) If we keep Major/Minor mode we can write

distro >= rhel-6.7
distro >= rhel-7.3
distro >= rhel-8.2

And it will work. Without minor mode - e.g. rhel-8.1 will match two (but shouldn't match any) and rhel-6.8 will match one (but should match)

AloisMahdal commented 3 years ago

I thought we decided to go with the default being continue: True

I'd vote for False. As I said, the advantage of first-match is it's much simpler to maintain: 1. you don't need to read the whole thing to get a picture of every parameter, 2. it's not so easy to break later clauses by editing earlier.

And that's something I'd go for as maintainer of the meta-data. Especially if my maintenance is measured in adding/removing distros (and keeping those that work intact) rather than adding/removing/refining parameters.

Do we want/need to preserve the major/minor mode comparison the same way?

IMO it's counterintuitive to have >= mean something else than in virtually any other language. (Despite the fact that there might be audience that is used to this.)

I can understand that someone wants this behavior, but then why not use different operator for different meaning? (>. comes to mind --- which could be easy to s/// --- but really, there are infinitely many options.)

pvalena commented 3 years ago 
distro >= rhel-6.7
distro >= rhel-7.3
distro >= rhel-8.2

From the user perspective I expect this to mean that the the test will run if one of the conditions is true, i.e. all of those could be merged into distro >= rhel-6.7. Does that make sense? Should I have other expectations?

Note: I mean in general, I understand that RHEL+CentOS can be a special case. But it can be confusing anyway.

lukaszachy commented 3 years ago 
distro >= rhel-6.7
distro >= rhel-7.3
distro >= rhel-8.2

From the user perspective I expect this to mean that the the test will run if one of the conditions is true, i.e. all of those could be merged into distro >= rhel-6.7. Does that make sense? Should I have other expectations?

The issue is that: Test is relevant for RHEL-6 family from rhel-6.7 and above, for RHEL-7 from 7.3 and for RHEL-8 from 8.2. That means test is NOT relevant for e.g. rhel-7.0 or rhel-8.1 So distro >= rhel-6.7 can't match for rhel-8.1

And this group of rules is very common in RHEL world, where more families are being release in parallel.

psss commented 3 years ago

Regarding the continue default I would agree with @AloisMahdal and keep it False. My impression/estimation is that the first-rule-wins use case will be more frequent. And it's also kind of backward compatible with the old relevancy.

And as far as minor distro comparison is concerned, it seems we should keep the backward compatibility there as the use case of adding a new feature / fixing bug in the middle of the release is quite common. Not sure about introducing a new operator. I'd rather suggest to stick with the old one.

AloisMahdal commented 3 years ago

@lukaszachy: [...] And this group of rules is very common in RHEL world [...]

Yes. But using <= for something other than rest of the world is counter-intuitive nevertheless.

@psss: [...] the use case of adding a new feature / fixing bug in the middle of the release is quite common.

Of ~5000 RHEL packages how many do this in a release? What about Fedora? Assuming TMT is going to be used outside RHEL, how many it would be on other distros?

Or better: when someone is going to read TMT meta-data expression like distro >= rhel-8.4, what is the chance that they will find it apparent that it does not include rhel-9.*? (BTW, does distro >= fedora-34 include fedora-35?)

Not sure about introducing a new operator.

But are you sure about introducing new meaning of a well-known operator? :-)

Compatibility should be preserved within a platform, but porting relevancy to new platform is probably the best opportunity to change it. (And with new operator the fix could be really simple.)

bocekm commented 3 years ago

Hi. I'm adding my use case with a plea to implement this feature :)

In convert2rhel (especially since this PR), I would like to run an fmf plan for unit tests through packit on three targets - epel-6, epel-7, epel-8. The first two targets have py2, the third has py3. And based on the python version I need to install specific dependencies. Currently there seems to be no way to specify under what conditions the plan should be executed.

What would definitely help is the relevancy of an fmf plan. E.g. 

relevancy:
    - "distro = rhel-6, rhel-7: True"

which would make sure the plan is executed only on RHEL 6 and 7. Well, in our case the epel targets are not RHEL but CentOS, but I can imagine this rhel-x relevancy could work for CentOS as well.

lukaszachy commented 3 years ago

Compatibility should be preserved within a platform, but porting relevancy to new platform is probably the best opportunity to change it. (And with new operator the fix could be really simple.)

Good point, we have very good opportunity to add new operator. It can be used automatically in the conversion for "distro" and "product" dimensions so ti will behave as before.

lukaszachy commented 3 years ago

Update: We had quite nice brainstorming with @psss and the outcome is:

t184256 commented 3 years ago

Will there be comments? We have a lot of short (40-200 char) explanations of why is something disabled and it's not clear where will those end up.

lukaszachy commented 3 years ago

Will there be comments? We have a lot of short (40-200 char) explanations of why is something disabled and it's not clear where will those end up.

Good point, we have to support comments.

psss commented 3 years ago

For commenting relevancy rules we can use just the yaml comment syntax, right? Or do we need to have them available in the API as well.

t184256 commented 3 years ago

Not sure how useful it is to query those.

I just noted that this is a piece of metadata we currently have as comment embedded inside relevancy. Now it's somewhat queriable, and moving it to yaml comments would make it even less queriable. We have to migrate it somewhere, and if there was a some dedicated because: where I could put it, the migration would be less lossy.

psss commented 3 years ago

I think it would be ok to support something like an optional comment field. What is the usual format of those comments? At the end of the line with the test case relevancy rule? Or above? Or both?

t184256 commented 3 years ago

Current usual is a comment line inside relevancy. The vast majority is a separate single short comment line, acting as a header for a rule or a group of them, something like this:

 relevancy: |
   # feature X won't make it to Fedora 32 at the earliest
   distro < fedora-32: False
   # feature X will be deprecated since Fedora 34, long live feature Y
   distro >= fedora-34: False
lukaszachy commented 3 years ago

So we will be adding an optional 'comment' attribute of string type, right? E.g.

rule:
 - environment:
      Foo: bar
   when: distro < rhel-8
   comment: |
     Long explanation why we this rule is necessary.
   continue: False
t184256 commented 3 years ago

Yes, you've captured it right.

AloisMahdal commented 3 years ago

But why, acually? What is the use case? If you want to enable inspection in reports et al., it's IMHO better served by linking to the original YAML (or including it in some <pre/>, maybe even along with test code) anyway.

comment object is already much more limited: you can't easily comment on just part of the rule, it doesn't do anything for non-rule things, and finally, it assumes that the comment actually answers (correctly!) what the inspector needs to know. (My gut tells me that it's going to mislead more often than not.)

t184256 commented 3 years ago

@AloisMahdal: I just don't like lossy data migrations =)

AloisMahdal commented 3 years ago

@t184256 if comment was a data object in the original relevancy then that's another story & my bad I missed it. Lossy is a no-go.

(It could be still worth considering getting rid of it for reasons above & supported by my earlier comment

Compatibility should be preserved within a platform, but porting relevancy to new platform is probably the best opportunity to change it.

)

psss commented 3 years ago

I think it's ok to migrate the old data into the attribute value. Those who would prefer yaml comments can still use them. So we would have three reserved words: when, continue and because. After thinking about it a bit I believe there is a lower chance that somebody would need to use because than use comment as the attribute value.

psss commented 3 years ago

The fmf part of the implementation is done:

I will take care of implementing this on the tmt side.