Eliminate the need for run() wrapper in the test-suite #3447
Conversation
The buildhost gets centrally set to "testhost" nowadays, there's no need to do anything in this test. And, turns out this hasn't actually been honored, the built packages get "testhost" from the upper level settings as it is. This confused us greatly.
|
Oh pooh, ONE test fails and I can't reproduce that locally. Going to be fun chasing this down... |
pmatilai
force-pushed
the
norun-pr
branch
2 times, most recently
from
ab7ab9e to
9f6c97b
Compare
|
Okay... So that "rpmdb --export and --import" test fails due to the --dbpath definition being removed. What's peculiar is that said test explicitly sets dbpath by itself: What makes it even more entertaining is that this PR passes "make check" and "make ci" for me locally, so it seems like one of those "only on Ubuntu" gotchas. I won't be able to finish this today, but I pushed the full PR back as originally submitted and converted to draft. |
run() overrides --dbpath so this hasn't been actually used at all. Nor is it needed - the underlying image is mounted read-only at this point as per the comment. Obviously --dbpath would affect the contents of the rpmdb found in that place, but for the purposes of this test it doesn't matter: we only care that it's on a read-only mounted media, which both /data and the system rpmdb are. However for some mystery reason, trying to open the database in /data/ fails on the GH Ubuntu runners, but succeeds when running on a Fedora host. This doesn't seem worth chasing...
This is technically covered by the reproducability tests but "testhost" is easier to reason about than an opaque checksum telling you "its wrong and YOU broke it"
There may already be something important in there...
No functional changes as such, but one step closer to being able to drop some of the wrapper functions entirely.
These were absolutely crucial in the fakechroot days but the test-environment is a very different animal now...
The snapshot case always needs a wrapper function of some kind, so it makes sense to optimize the setup for the non-snapshot case to allow running commands without any wrappers in the way. Set the test-suite %_tmppath as per non-snapshot case and move the last remaining command line --define override to runroot() instead. This makes run() entirely redundant. The %_tmppath override in runroot() shouldn't really be necessary but scriptlet related tests fail without that. I suspect it's related to /tmp being tmpfs in the test-suite but dunno. It's a mystery for some other day...
All tests are running in a container with the newly built rpm as the system rpm, for many things runroot is simply no longer needed. This isn't anywhere near them all, just a bunch of the more obvious ones. Running this without the extra nested root does have the downside of exposing testsuite paths in the container, eg the second rpmspec --parse test is left as runroot here because it'd otherwise produce output like "/srv/rpmtests.dir/195/tree/build/SOURCES/hello-1.0-modernize.patch" and we don't want to have to adjust numbers in unrelated tests if we add something before that. This could probably be hidden with further container magic.
Drop runroot from most macro-related tests. There are any number of further test-cases where doing so would be possible but these are among some of the more obvious ones.
See previous commits for rationale.
|
There are about a thousand further cases were runroot() can be eliminated but lets leave something for another rainy day 😅 |
|
As for the "Ubuntu mystery", see 06e4da1 - I still don't know why exactly its failing for that one path but not for something else that should be on the same image, but for the purpose of that test which of the two paths gets used is wholly irrelevant, so I'm not going to die on that hill today. |
|
Looks good, well done 😄 The courage for diving into this bottomless pit is commendable (and very much appreciated) 😄 |
Details in commit messages, but short summary: move the test-suite rpm configuration from --define's to a permanent file, and setup_env calls from the wrapper functions to RPMTEST_CHECK*(). As a result, run() becomes entirely redundant can be removed - that is, for a large class of tests, you can just use commands directly without any extra fubar.