teemtee / tmt

Test Management Tool
MIT License
76 stars 112 forks source link

Install dependencies of upgrade tasks #2866

Closed martinhoyer closed 1 week ago

martinhoyer commented 1 month ago

Fixes #2643.

Pull Request Checklist

psss commented 2 weeks ago

@martinhoyer, could you please include a simple test checking that beakerlib would be installed even if the test which is executed is using shell as the framework? I'd suggest to create a slightly modified copy of /tests/execute/upgrade/simple which would select only the shell test:

https://github.com/teemtee/tmt/blob/159b37cd81ef38bc89fd943476a0b263a294d2e5/tests/execute/upgrade/simple.sh#L13-L18

lukaszachy commented 2 weeks ago

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in https://github.com/teemtee/tmt/issues/2643#issuecomment-1959707863 it is quite fast. We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

martinhoyer commented 2 weeks ago

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in #2643 (comment) it is quite fast. We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

Thanks @lukaszachy , I'm trying to adapt your reproducer, but I'm not super sure how to modify it so it passes the tmt lint.

btw, I've found a small dummy-test-package-crested package to add as require in the upgrade task: Total download size: 11 k; Installed size: 6.9 k

lukaszachy commented 2 weeks ago

I'm trying to adapt your reproducer, but I'm not super sure how to modify it so it passes the tmt lint.

It smells with linter bug. I have same 'warn' reported for in plans for /tests/execute/upgrade (in /data). Especially the warn C000 value of "how" is not "tmt" check...

psss commented 2 weeks ago

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in #2643 (comment) it is quite fast. We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

Yes, and /tests/execute/upgrade/simple actually does not run the whole upgrade as it selects only /tasks/prepare from the upgrade repo which does almost nothing. My suggestion was to just do something like this:

tmt run --all -v
    plan --name /plan/no-path
    test --name /test/shell
    provision --how container --image fedora:39
    execute --how upgrade --test /tasks/prepare

Which should be fast enough and would verify that beakerlib is installed as the upgrade task requires it but /test/shell does not.

psss commented 1 week ago

/packit test

psss commented 1 week ago

IMO also worth of release note

Yeah, makes sense, added one in 376efe07 with some minor adjustments.

mmacura311 commented 1 week ago

sanity tested this functionality with following copr builds:

tmt-1.33.dev29+g2879190a-main.fc40.noarch
tmt+export-polarion-1.33.dev29+g2879190a-main.fc40.noarch
tmt+report-junit-1.33.dev29+g2879190a-main.fc40.noarch
tmt+report-polarion-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-beaker-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-container-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-virtual-1.33.dev29+g2879190a-main.fc40.noarch
tmt+test-convert-1.33.dev29+g2879190a-main.fc40.noarch
tmt+all-1.33.dev29+g2879190a-main.fc40.noarch

did run an upgrade test that require beakerlib in the pre-upgrade test already at the beginning, and second one where requirement is added only at the upgrade phase. Both tests passed.