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

Running upstream tests in Fedora dist-git needs upstream tarball discovery or flexible ref: -- still needs #978 #585

Closed
martinpitt opened this issue Feb 25, 2021 · 20 comments · Fixed by #742
Assignees
Labels
discover Discover step must high priority
Milestone

Comments

@martinpitt
Copy link

cockpit-podman now has an FMF test plan which runs in packit, which works very nice. I am currently investigating how to run the exact same tests downstream in Fedora. (Thanks to @psss for your great help!).

plans/upstream.fmf currently looks like this:

discover:
  how: fmf
  repository: https://github.com/cockpit-project/cockpit-podman
  ref: "29"
execute:
  how: tmt

But this is awkward: I don't want to manually bump the ref: "29" each time there is an upstream release. This is error prone and breaks automation. The default value of ref: is master, which is not suitable at all -- upstream's mainline tests are pretty much guaranteed to fail on the last or even earlier release. discover does not currently have any other method.

So one way to fix this would be to support a shell command, or even provide that in tmt itself -- in our case, we'd parse Version: from the spec and requests that as a tag.

But even that is a bit of a crutch -- having to check out a remote git tree is a point of brittleness, and this shell hackery is too much indirection. What I'd really like to do is what STI does: It unpacks the upstream tarball into $WORKDIR/source, and tests can just run there. See cockpit-podman's STI playbook role for example.

It would be really nice to retain that semantics with FMF as well. The Fedora dist-git should be a small skeleton only, all the interesting bits should be done, gated, and released upstream. To keep TMT agnostic to the dist-git packaging mechanics, I could imagine a prepare-discover: or pre-discover: shell command which could then do

prepare-discover: |
    fedpkg sources
    tar xf cockpit-podman*.tar.?z
discover: 
    how: fmf
    path: cockpit-podman/

This would actually also help @jscotka with his efforts of dynamically generating FMF metadata from Python unittests -- that step could then run the generator, so that the subsequent discover step can pick them up.

This is actually very similar to prepare:, but that runs after discover.

@psss
Copy link
Collaborator

psss commented Mar 11, 2021

Yes, this seems to be a demanded feature. Let's try to find a solution for this soon. Proposing for 1.5.

@psss psss added this to the 1.5 milestone Mar 11, 2021
@psss psss added the discover Discover step label Mar 11, 2021
@psss
Copy link
Collaborator

psss commented Mar 11, 2021

Possible temporary workaround I'm using for referencing tmt integration tests from fmf is to set up a branch which contains tests relevant to the latest released bits:

So the only thing I need to do when releasing a new tmt is to move forward the fedora branch.

@jkonecny12
Copy link

jkonecny12 commented Mar 12, 2021

Hi @psss, your solution is great. However, it has a problem that the tests will fail during time between upstream and downstream release is done. In that period the upstream test branch will point to the tests which are not yet in gating.

@psss
Copy link
Collaborator

psss commented Mar 12, 2021

Yes, yes, just a workaround, not very perfect... ;-)

@jscotka
Copy link
Collaborator

jscotka commented Mar 18, 2021

@psss as Martin referenced my issue #588 it could be also more generic and very useful. Not just about getting some version of test before discovery.

martinpitt added a commit to cockpit-project/cockpituous that referenced this issue Mar 19, 2021
As of today, Fedora dist-git FMF test plans have to specify a git repo
and fixed ref for where to find the tests to run.
(teemtee/tmt#585)

We don't want to bump this manually on every release, so teach
release-koji to do that automatically for every old version that appears
in plans/*.fmf.

This can be dropped once there is a way to just run the tests from the
upstream tarball in the dist-git, or run any "pre-discover" code.
@martinpitt
Copy link
Author

My hack until then is to have our release scripts bump the ref automatically.

martinpitt added a commit to cockpit-project/cockpituous that referenced this issue Mar 22, 2021
As of today, Fedora dist-git FMF test plans have to specify a git repo
and fixed ref for where to find the tests to run.
(teemtee/tmt#585)

We don't want to bump this manually on every release, so teach
release-koji to do that automatically for every old version that appears
in plans/*.fmf.

This can be dropped once there is a way to just run the tests from the
upstream tarball in the dist-git, or run any "pre-discover" code.
@jkonecny12
Copy link

@psss as Martin referenced my issue #588 it could be also more generic and very useful. Not just about getting some version of test before discovery.

I like that solution. As you said it's more general.

@pvalena
Copy link
Collaborator

pvalena commented Mar 26, 2021

I always thought this is where https://src.fedoraproject.org/tests/***/ comes into play. Yes, it's similar to maintaining an upstream branch, but I think that's the point- having the ACLs in the OS repo (for tests run on the OS CI), not upstream ACLs.

I can see your point, when you're in charge of upstream, that you dont want your tests duplicated. But this basically generates tests dynamically from arbitrary upstream source... which I think is an "unsafe" operation, when I simply want to "run" the tests, not modify a host OS. I'd be fine with this running in a VM, f.e., but that's a lot of overhead for one archive...

(I wrote more on this in #588.)

@martinpitt
Copy link
Author

@pvalena: For me this is exactly not the point -- I want to maintain and run the tests during upstream CI, so having this separate tests/ namespace is useless, too expensive [1], and error prone. The nice thing about STI is that you can effortlessly run tests from the upstream release tarball, which I consider the right thing to do. I"m not much fussed about how exactly to get this back (unpacking the source tarball in tmt, or mapping the spec version to a tag, or something equivalent).

[1] in the sense of keeping gating tests actually working -- if they only run downstream, they always fail at the wrong time, and are nothing but a nuisance.

@martinpitt
Copy link
Author

But this basically generates tests dynamically from arbitrary upstream source... which I think is an "unsafe" operation, when I simply want to "run" the tests

No, not really - I want to take the tests as is from the upstream source which is defined in the spec. This is "safer" than tmt checking out some arbitrary git repo, as it does now. It's also awkward that test discovery has to call out to the internet in the first place, while it already has everything that it needs locally and validated.

@jkonecny12
Copy link

jkonecny12 commented Mar 29, 2021

In general you won't get much adaption if there will be more work to be done manually like syncing upstream tests to another downstream repo. Also why would people use TMT if it would gave them more work than STI which they have now.

Honestly, I think the whole downstream tests repository is a wrong idea from my PoV. I know some teams would benefit that but in most cases you want to use upstream tests and work on them not having copy downstream.

@psss psss added the must high priority label Apr 8, 2021
@martinpitt
Copy link
Author

Reading #588 and the replies here, I do understand your concern about running too much code in the prepare step on the host machine. For this specific issue, I'd really be content with discovering the tests not from a git repo, but a local tarball or dir:

discover:
  how: fmf
  archive: dist-git

... or something like that. That could invoke {fed,cent,rh}pkg sources to download the tarball, unpack it, and discover tests from there? Admittedly this is rather specific to RPM packaging, but one way or the other we need to support that.

The standard-test-roles standard-test-source playbook should be a good inspiration, as that pretty much did the right thing in the last years.

@lukaszachy
Copy link
Collaborator

I need *pkg source too. However I wouldn't bind it to how:fmf and keep it a separate thing. Reason - test might not be in fmf but e.g shell, or later pytest (or any future how:plugin).

@psss psss modified the milestones: 1.5, 1.6 Apr 27, 2021
@lukaszachy lukaszachy self-assigned this Apr 27, 2021
@lukaszachy
Copy link
Collaborator

lukaszachy commented Apr 27, 2021

In addition to how:fmf I'd like to have possibility to use also other how plugins (currently just shell)

discover:
   how: shell
   archive: dist-git
    tests:
    - name: run check
      test: make check

Imho we can make archive (or better name) common option for discover plugins and for dist-git value it would:

  1. Run fedpkg sources in cwd
  2. Run rpm-build -bp on fetched sources
  3. Finally do 'how' on outcome of previous step.

Would that work for you @martinpitt too?

@martinpitt
Copy link
Author

@lukaszachy Sure! That's pretty much what I asked above as well.

@kkaarreell
Copy link
Collaborator

Just a thought on the rpmbuild -bp approach mentioned above. This step involves a check that all BuildRequires are installed (this could be skipped with --nodeps). While some of those packages may be really necessary for running actual tests, it may not always be the case. In fact, installing unnecessary BuildRequires would probably result in requiring CRB and Buildroot repos and due to having too many non-RHEL-supported packages installed possibly partially invalidate test results (like a test could be passing just because a non-distribution package is present, although it should not be required).
I am thinking whether an option to go the --nodeps way and manually specifying test requires in FMF won't be safer.

@psss psss modified the milestones: 1.7, 1.8 Jul 29, 2021
@lukaszachy lukaszachy modified the milestones: 1.8, 1.9 Sep 29, 2021
@lukaszachy
Copy link
Collaborator

Hopefully Autumn will bring more time to work on this. Moving to 1.9

lukaszachy added a commit that referenced this issue Oct 12, 2021
Based on meeting discussion and #875
Understands Fedora sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 12, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 12, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 19, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 19, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 19, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Oct 19, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Nov 18, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Nov 18, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
lukaszachy added a commit that referenced this issue Nov 18, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
@psss psss linked a pull request Nov 22, 2021 that will close this issue
@psss psss closed this as completed in #742 Nov 22, 2021
psss pushed a commit that referenced this issue Nov 22, 2021
Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
@martinpitt
Copy link
Author

Ooh, I somehow missed that this got fixed, nice! I tried this in https://src.fedoraproject.org/rpms/cockpit-podman/pull-request/37 , but it fails with

Found 1 plan.

/plans/upstream
    discover
        how: fmf
        directory: /var/ARTIFACTS/workdir-repository-None-9LLabI
        hash: 78bf01e
        summary: 0 tests selected
        warning: No tests found, finishing plan.

https://tmt.readthedocs.io/en/stable/spec/plans.html doesn't say a lot about this new option, but the example looks like that's all required?

@lukaszachy
Copy link
Collaborator

I'm still working on fixing this - Just guessing, but isn't this affected by #978 ?

@martinpitt
Copy link
Author

@lukaszachy oh right, thanks for the pointer -- that was the reason why I ignored the closing of this issue last time. Cheers!

@martinpitt martinpitt changed the title Running upstream tests in Fedora dist-git needs upstream tarball discovery or flexible ref: Running upstream tests in Fedora dist-git needs upstream tarball discovery or flexible ref: -- still needs #978 May 24, 2022
martinpitt added a commit to martinpitt/cockpit that referenced this issue May 24, 2022
Fedora dist-git FMF test plans have to specify a git repo
and fixed ref for where to find the tests to run. This is bad, and at
some point it should instead just discover the tests from the upstream
source tarball instead. Until teemtee/tmt#585
gets fixed, we need to bump `ref:` in dist-git's plans/upstream.fmf.

Cockpituous had a hack for this [1], which we need to transplant to
packit now. Unfortunately this reintroduces `files_to_sync:`, but all of
this will go away when the follow-up issue
teemtee/tmt#978 gets addressed.

Create the file in tmp/ instead of plans/ (from where it would be easier
to sync) as packit refuses operation on an unclean tree. tmp/ is in
.gitignore.

[1] cockpit-project/cockpituous@2ef3f6c9991
mvollmer pushed a commit to cockpit-project/cockpit that referenced this issue May 24, 2022
Fedora dist-git FMF test plans have to specify a git repo
and fixed ref for where to find the tests to run. This is bad, and at
some point it should instead just discover the tests from the upstream
source tarball instead. Until teemtee/tmt#585
gets fixed, we need to bump `ref:` in dist-git's plans/upstream.fmf.

Cockpituous had a hack for this [1], which we need to transplant to
packit now. Unfortunately this reintroduces `files_to_sync:`, but all of
this will go away when the follow-up issue
teemtee/tmt#978 gets addressed.

Create the file in tmp/ instead of plans/ (from where it would be easier
to sync) as packit refuses operation on an unclean tree. tmp/ is in
.gitignore.

[1] cockpit-project/cockpituous@2ef3f6c9991
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discover Discover step must high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants