packit-service icon indicating copy to clipboard operation
packit-service copied to clipboard

Passing parameters to tmt

Open Venefilyn opened this issue 3 years ago • 11 comments

When it comes to tmt, you have the ability to pass name, filter to reduce the test suite overall. But this is not something you can pass when doing /packit test. It would be great to have the ability to use filters this way so that we can have a limited plan running for every PR but trigger different plans depending on the command sent

If we have to test for kernel modules in a specific PR, I'd want the ability to do /packit test --filter "tag:kmods" and have integration tests started for that

Venefilyn avatar Nov 11 '22 15:11 Venefilyn

Thank you for opening the RFE!

Would such a test run be required to pass in order to merge a pull request?

We can easily implement the command arguments and forward them to TF - the untrivial part is the reporting: what would be the name for the check (should it be a new one? should we rewrite the generic?); how should these be treated when a new push happens, etc.

TomasTomecek avatar Nov 16 '22 11:11 TomasTomecek

Would such a test run be required to pass in order to merge a pull request?

I'd say it depends. I don't want to run all the integration tests we have for each PR, it's becoming too much for that. If we reduce to having a limited test suite for PRs, we'd want to run all of the integration tests for the main branch instead. But ideally, if a feature is touching kmods or similar, it would be a nice to have feature to be able to run tests that hasn't been run before in the PR based on a filter

That being said, I don't think it's a requirement for merging PRs. But it does give ease of mind when a PR is a bit more complex to be able to run specific tags etc.

what would be the name for the check (should it be a new one? should we rewrite the generic?)

IMO it should be a new one if possible, run existing plans with the filter and possibly prepend the filter to the name

how should these be treated when a new push happens

If the filter is more for creating new checks that run. I wouldn't have it run on consequent commits, I don't think it would make sense as you it's an extra thing for ease of mind rather than a requirement. But maybe it's possible to allow for both somehow? Labels? Different command? /packit test-filter-persistent "tag:kmods" and /packit test-filter "tag:kmods" for instance

Venefilyn avatar Nov 16 '22 12:11 Venefilyn

Would such a test run be required to pass in order to merge a pull request?

I'd say it depends. I don't want to run all the integration tests we have for each PR, it's becoming too much for that. If we reduce to having a limited test suite for PRs, we'd want to run all of the integration tests for the main branch instead. But ideally, if a feature is touching kmods or similar, it would be a nice to have feature to be able to run tests that hasn't been run before in the PR based on a filter

That being said, I don't think it's a requirement for merging PRs. But it does give ease of mind when a PR is a bit more complex to be able to run specific tags etc.

Makes sense to me. So it would be an interface to run a specific set of tests in CI for your convenience: use upstream pull request as a venue to integrate the work rather than running those tests locally & set up the environment manually. Agreed, I think that with Packit and Testing Farm, we're heading in this direction of utilizing GitHub and services to make smooth development experience.

how should these be treated when a new push happens

If the filter is more for creating new checks that run. I wouldn't have it run on consequent commits, I don't think it would make sense as you it's an extra thing for ease of mind rather than a requirement. But maybe it's possible to allow for both somehow? Labels? Different command? /packit test-filter-persistent "tag:kmods" and /packit test-filter "tag:kmods" for instance

Oki, let's treat this as a subsequent RFE.

Thank you Eric for the clarifications! I applied the "discuss" label to discuss this at the next Packit architecture meeting.

TomasTomecek avatar Nov 16 '22 12:11 TomasTomecek

Related to https://github.com/packit/packit-service/issues/1627

TomasTomecek avatar Dec 06 '22 10:12 TomasTomecek

Maybe custom tmt context can help with the filtering now when https://github.com/packit/packit-service/issues/1797 has been implemented.

kkaarreell avatar Mar 14 '23 09:03 kkaarreell

This feature would be very useful. We discussed something similar today with @lukaszachy and agreed it would be nice to have something like an alias for an extra job which would hold additional configuration. The config could look like this:

- job: tests
  name: full
  manual: true
  trigger: pull_request
  targets:
    - fedora-all
  tf_extra_params:
    environments:
      - tmt:
          context:
            how: "full"

This test job would not be executed by default but would be manual, triggered using the following comment:

/packit test full

Results would be reported as a separate check using the provided name. This would allow to define additional tests jobs for common scenarios and easily trigger them when desired using a concise command. Thoughts?

psss avatar Apr 17 '23 11:04 psss

On the tmt hacking session a good point was raised that manual: true sounds a bit strange when used together with trigger: pull_request. What about trigger: manual? Would that make a bit more sense?

- job: tests
  name: full
  trigger: manual
  targets:
    - fedora-all
  tf_extra_params:
    environments:
      - tmt:
          context:
            how: "full"

Could these manual checks be displayed in the list of checks as idle or something similar so that they are not overlooked?

psss avatar Apr 24 '23 09:04 psss

Or what about pull_request_manual, assuming some PR related metadata may be required.

kkaarreell avatar Apr 24 '23 10:04 kkaarreell

Hello all!

So to clarify. Instead of an argument for comment command, it's more practical to trigger a specific job that is already configured (and that is somehow marked as manual).

@SpyTec would that work for you as well?


Implementation details:

  • [x] We can use identifier as a job name.
  • [ ] We might want to use a new job config option so we can use it for all the jobs (the same has already been asked for propose-downstream).
  • [ ] ~~When parsing the jobs, we need to skip running the job when manual: true is set.~~ (Will be implemented in #1806)

lachmanfrantisek avatar Apr 26 '23 07:04 lachmanfrantisek

Also, is anyone willing to help us with the implementation? (We can definitely help and provide pointers.) I really see value in that but we don't have much team capacity to spare so I can't promise any dates because of that...

lachmanfrantisek avatar Apr 26 '23 07:04 lachmanfrantisek

As for the trigger naming, some proposals are also in #1806.

lbarcziova avatar Apr 26 '23 09:04 lbarcziova