main: unif/refactor std{out,err} test handling #416
Conversation
When we generate the osbuild manifest we need to take a bunch of commandline options into account. These are collected and passed via `manifestOptions` in the CLI. There is a (small) overlap with options that are then "converted" to options that need to be passed to `manifestgen` via `manifestgen.Options`. Previously there was manual code for this but its slightly nicer to just embedd the `manifestgen.Options` into the more general `manifestOptions` of the CLI. This avoid some boilerplate code and might serve as a useful pattern in other places. So this commit does that now (even though the wins are not that big).
We recently had two issues where creating our buildroot was broken in certain situations because the local rpm would not be compatible with the requirements of the package that got installed in the buildroot: https://issues.redhat.com/browse/RHEL-128741 osbuild#413 The issue here is that the local rpm is something we do not control but we need a way to "bootstrap" our buildroot (once we have a buildroot we use that for everything else). This commit enables the "bootstrap" container feature of the images library by default to avoid this dependency. This means that we "podman pull" a minimal container (e.g. ubi for rhel) that contains python3 and rpm to then use it to install our real buildroot. Note that this enables it all the time even if the host distribution and target distribution match. The reason is rebuildability - ie. the same manifest should always produce the same result and in the general case we do not know if e.g. a manifest that was part of `image-builder build --with-manifest` is used somewhere else again. For restricted environment where pulling a container is a problem or for situations where its known that the host rpm is fine we provide: `--without-bootstrap-container` to disable this function.
For historic reasons we have two way to test the std{out,err}
output. One is to just monkey patch `var osStdout = os.Stdout`
and then replace it in the tests, the other to have a helper
that works like a `with` in python that replaces the stdout/stderr
dynamically, e.g.:
```go
stdout, stderr := testutil.CaptureStdio(t, func() {
err = main.Run()
require.NoError(t, err)
})
```
The trouble is that they are not quite compatible. In places
where we use the monkey patches `osStdout` the
testutil.CaptureStdio() will not work as it (temporarely)
replaces the "real" `os.Stdout` (and vice-versa).
So this commit unifies all tests to use testutil.CaptureStdio()
which feels slightly nicer than the other approach in the
sense that one does not need to remember to use `osStdout` in
the code. Its mostly mechanical.
lzap
left a comment
lzap
left a comment
There was a problem hiding this comment.
I mean it is better, but I really do not like mocking global functions like that. These tests should have not been unit tests but fully integration tests actually running the binary via exec syscall. This way, we would not need to worry about this, every test would have been a new process.
But still that is a workaround to the main problem we have in the design here: any functionality that does print anything to stdout/stder should have a parameter with io.Writer so we can test anything we want.
This is for another day tho, your change is for the good, thanks :-)
When this was written it was a bit unclear if we want to embrace pytest based integration tests. It seems we have landed on this so most of these could just be written in the style of the current
Well, its more of a trade-off than a "problem". When testing a cli tool that always prints to stdout/stderr, when adding an extra writer (well two actually for both stdout/stderr) makes the common case (stuff is not run under testing) look less nice, hence this optimizes for this (on the expense of the test setup that you dislike). But of course one can decide a different trade-off if the values/weights are different. |
2 participants
[draft as this is build on top of https://github.com//pull/415 and there would be conflicts otherwise, if 415 does not go in I think its still worth doing this (it just requires to remove the one extra test added in 415]
For historic reasons we have two way to test the std{out,err}
output. One is to just monkey patch
var osStdout = os.Stdoutand then replace it in the tests, the other to have a helper
that works like a
within python that replaces the stdout/stderrdynamically, e.g.:
The trouble is that they are not quite compatible. In places
where we use the monkey patches
osStdoutthetestutil.CaptureStdio() will not work as it (temporarely)
replaces the "real"
os.Stdout(and vice-versa).So this commit unifies all tests to use testutil.CaptureStdio()
which feels slightly nicer than the other approach in the
sense that one does not need to remember to use
osStdoutinthe code. Its mostly mechanical.