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

Discover from dist-git source should explore metadata under the source directory #978

Closed
psss opened this issue Dec 13, 2021 · 14 comments
Closed
Assignees
Labels
priority | must high priority, must be included in the next release step | discover Stuff related to the discover step
Milestone

Comments

@psss
Copy link
Collaborator

psss commented Dec 13, 2021

Currently the source tarball is unpacked under the discover/default/tests directory and test discover is done from there. This brings two problems:

  • When the source tarball includes the .fmf directory it is ignored as a nested fmf.Tree.
  • Discovered tests names receive an extra prefix from the source directory

We should discover tests directly from the unpacked directory and make sure that it has metadata tree root defined, e.g. call tmt init if needed.

@martinpitt ran into this problem when working on this cockpit pull request.

@psss psss added the step | discover Stuff related to the discover step label Dec 13, 2021
@psss psss added this to the 1.10 milestone Dec 13, 2021
@psss psss added the priority | must high priority, must be included in the next release label Dec 13, 2021
@lukaszachy lukaszachy self-assigned this Dec 13, 2021
@lukaszachy
Copy link
Collaborator

Thank you for the report, I'll try to create a fix this week.
Proposal: Expectation is that extracted source contains .fmf root. When it is not there tmt will do tmt init in extracted sources.

New option --dist-git-init will change the behaviour:

  • False -> disable feature (don't run tmt init at all),
  • str -> relative path in extracted sources where tmt init should be called,
  • True -> call tmt init in the root of extracted sources [default]

@psss
Copy link
Collaborator Author

psss commented Dec 13, 2021

The --dist-git-init feature drafted above looks like a very elegant solution to me. Thanks!

@bcl
Copy link

bcl commented Jan 28, 2022

I have a possibly separate problem with dist-git-source, I expected it to act like it did in the STI tests and extract the source and then run my test script, but it seems to be extracting the source, not finding any tests, and then not running the execute section. The test has already been found, that's what it is running, so why is execute skipped?

@lukaszachy
Copy link
Collaborator

I expected it to act like it did in the STI tests and extract the source and then run my test script,

That is a bit different I'm afraid. In that case you are not expected to search for any tests in the extracted sources, right?
So dist-git-source just adds code but metadata are already present.
I think it could be quite easy to add.

@bcl
Copy link

bcl commented Jan 31, 2022

I think I figured it out. The differences between plans and tests aren't obvious and I got it working by putting the actual test execution into tests/unit.fmf and the setup with dist-git-source into a corresponding plan under plans/

In that case you are not expected to search for any tests in the extracted sources, right?

Right, the tmt tests are in dist-git.

@lukaszachy lukaszachy modified the milestones: 1.10, 1.11 Jan 31, 2022
@psss
Copy link
Collaborator Author

psss commented Feb 11, 2022

Just realized one aspect which will probably make the implementation less straightforward: If I remember correctly, there can be multiple source tarballs included in the sources list, right? What shall we do in such case? We cannot just randomly choose where to perform the tmt init. Plus combining tests from both sources would need the metadata root to be in discover/default/tests directory.

@lukaszachy lukaszachy modified the milestones: 1.11, 1.12 Feb 22, 2022
@lukaszachy
Copy link
Collaborator

Needs more time to evaluate possible corner cases

@martinpitt
Copy link

Would it be possible to provide a solution for a single/first tarball, i.e. look at Source0 only? The vast majority of packages have only one tarball anyway, and even more only have tests in their "primary" source tarball. We used to have a hack in our cockpituous scripts, but with the world moving to packit (which does not have "nice" ways to arbitrarily hack files in dist-git) this becomes more awkward to work around.

@martinpitt
Copy link

For those following along, I created this hack and successfully tested it: https://github.com/cockpit-project/cockpit/pull/17378/files (but.. eww!)

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
@lukaszachy
Copy link
Collaborator

Could you try with #1128 please? I believe having more sources doesn't matter for that implementation.

@psss
Copy link
Collaborator Author

psss commented Jun 6, 2022

Should be covered by #1128.

@psss psss closed this as completed Jun 6, 2022
@martinpitt
Copy link

@psss, @lukaszachy : Sorry for the late reply, sprint week and long weekend. I'm super happy to see this, thanks! Does this still need a release, or does the Testing Farm use git? I. e. time to retry https://src.fedoraproject.org/rpms/cockpit-machines/pull-request/4# now or later? Thanks!

@psss
Copy link
Collaborator Author

psss commented Jun 7, 2022

I've just pushed out bodhi updates. It should get to the Testing Farm around tomorrow.

@martinpitt
Copy link

Thanks @psss! We collided, I rebased https://src.fedoraproject.org/rpms/cockpit-machines/pull-request/4 an hour ago, and it still failed. Nice! Then I'll retry with 1.14.0 once that lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority | must high priority, must be included in the next release step | discover Stuff related to the discover step
Projects
None yet
Development

No branches or pull requests

4 participants