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

[RFE] add option to print error/warning if some test case identifiers are not unique #49

Closed
akhait opened this issue Oct 1, 2018 · 7 comments
Assignees
Labels
invalid This doesn't seem right

Comments

@akhait
Copy link

akhait commented Oct 1, 2018

According to documentation, test case identifiers should be unique within current fmf structure. However, it is possible to work with a configuration text file, that has duplicated identifiers.

Example:

/python/Sanity/sys:
  component:
    - python3
  environment:
    PYTHON: /usr/libexec/platform-python
    PACKAGES: python3

/python/Sanity/sys:
  component:
    - python

Having that test.fmf, fmf show in the root directory shows

/Sanity/sys/test/CoreOS/python/Sanity/sys
component: python

so the first test was overwritten by the second one with duplicated identifier with no error returned.

As a user, I would like to be aware of the fact that I have duplicated test case identifiers in .fmf file, either with a visible warning or error and abortion of execution.

@psss psss self-assigned this Oct 2, 2018
@psss
Copy link
Collaborator

psss commented Oct 2, 2018

I see your point. But I'm afraid there is not much fmf could do with that as it is already yaml parser which processes the data and returns only the second duplicate. And I don't think we should implement our own parser of yaml files.

@psss psss closed this as completed Oct 2, 2018
@psss psss added the invalid This doesn't seem right label Oct 2, 2018
@AloisMahdal
Copy link
Contributor

Frankly the fact that duplicate (and inconsistent) ids were possible was one of the most annoying aspects of the old RHTS, that fmf should replace. So it's bit of a shame that fmf still allows that (although not formally.)

What @akhait reports, should be probably caught by some kind of fmf linter. (not that I do/will have skills or time to implement one, I'm just talking here :-))

@jakubkrysl
Copy link

@AloisMahdal yamllint looks like nice tool we could use here :)

@AloisMahdal
Copy link
Contributor

Well if the above is really not "good" YAML then yeah... I stand corrected and take back my rant (I'll save it for later.)

@psss
Copy link
Collaborator

psss commented Oct 3, 2018

Thanks for the pointer, @jkrysl. That would make it much more easier. Still I am not convinced it would be worth to bring this additional dependency. But I am not strictly against it. Feel free to convince me ;-)

One more thought which might be relevant: Elasticity, the core feature of FMF, expects that metadata for a single object with a unique identifier might be defined at several places.

So in this respect non-unique identifier is natural/acceptable for FMF. But of course, in the example @akhait provided it makes no sense and is just overwritten by YAML parser.

@jakubkrysl
Copy link

@psss I think we need stronger case to add this dependency. Now I see it as "nice to have", as it uncovers user overlook / error in single yaml file.

Plus I am a bit concerned with performance impact - scanning each file is extra step when building the tree. Does anyone know if this concern is valid and how big would that impact be? I think few % is still worth it, as debugging this sort of overlooks / errors takes quite some time.

@AloisMahdal How viable is adding this new dependency to remove "the most annoying aspects of the old RHTS, that fmf should replace"? Can we get some feedback from people involved in the process?

@AloisMahdal
Copy link
Contributor

I must admit I don't have strong case here; I got a bit on a rant; sorry for provoking without real case in hand. I even don't use fmf yet (I do plan on integrating it in my JATS framework but that is unfortunately on hold for now.)

The "annoying aspect" I meant was that RHTS allows just saying TEST=/foo/bar in the Makefile without any respect to the hierarchy, which makes it easy to create counter intuitive and confusing states just by neglecting to maintain the TEST when moving scripts around. Also it makes discovery unsafe because now it can be anything (I tried to write some meta-data traversal over that; it ends up working in only very basic cases.) The latter part FMF basically solves by using YAML. The former is more of fault of convention outside the suite level (ie. that TEST is taken as fully qualified, so I can redefine anyone's test id in the world; no questions asked.); it's not really for FMF to handle.

The case in OP seems a bit scary in that I see how it could lead to some headaches when maintaining larger collections. Imagine having lots of virtual ids (even deeper in tree), clashing with each other, and with "real" ids. This is pretty much reason why I'd just avoid virtual id's completely: just let all ids be defined by file hierarchy, and then tree(1) (and #26) is your friend. Once you start using the virtual ones, it can become messy really fast. Having the kind of help OP suggests, or yamllint, could help, but then again, I myself would probably not benefit from it as I would just completely stay away from the concept of virtual cases whatsoever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

4 participants