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

Add new Rust-based tests #2048

Merged

Conversation

cgwalters
Copy link
Member

There's a lot going on here. First, this is intended to run
nicely as part of the new cosa/kola ext-tests.

With Rust we can get one big static binary that we can upload,
and include a webserver as part of the binary. This way we don't
need to do the hack of running a container with Python or whatever.

Now, what's even better about Rust for this is that it has macros,
and specifically we are using commandspec
which allows us to "inline" shell script. I think the macros
could be even better, but this shows how we can intermix
pure Rust code along with using shell safely enough.

We're using my fork of commandspec because the upstream hasn't
merged a few PRs.

This model is intended to replace both some of our
make check tests as well.

Oh, and this takes the obvious step of using the Rust OSTree bindings
as part of our tests. Currently the "commandspec tests" and "API tests"
are separate, but nothing stops us from intermixing them if we wanted.

I haven't yet tried to write destructive tests with this but
I think it will go well.

tests/inst/src/repobin.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member Author

cgwalters commented Mar 30, 2020

Also @jlebon my intention if this works out is to factor out shared bits into a crate or so, then start migrating some of rpm-ostree's tests. There's some other prep work there that needs to happen, among them things like vm_build_rpm probably needs to instead be "build a set of test RPMs up front, sync them via deps/".

@@ -0,0 +1,2 @@
target/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
target/
/target

rust-lang/cargo#4944

@jlebon
Copy link
Member

jlebon commented Apr 16, 2020

Interesting stuff! I think using Rust to help with host tests is a natural fit. I have to admit though I find the intermix of Rust and shell-via-commandspec like that harder to follow. Maybe a middle ground is to keep the main path in shell, but use Rust as needed? So e.g. we'd call out to Rust for starting an HTTP server.

@cgwalters
Copy link
Member Author

I think using Rust to help with host tests is a natural fit. I have to admit though I find the intermix of Rust and shell-via-commandspec like that harder to follow.

I think I can make this bit more elegant still.

Maybe a middle ground is to keep the main path in shell, but use Rust as needed?

There are a few problems with that; it means for example we still end up needing a libtest.sh. And I just feel burned trying to write nontrivial logic in shell. Notice here the pattern of:

if ostree --should-fail 2>err.txt; then
  fatal "succeeded"
fi
assert_file_has_content err.txt 'someerror'

is encapsulated in a Rust API cmd_fails_with().

To be clear part of my goal here isn't just to use this approach for ostree - one can hopefully imagine something like initramfs code using this approach too.

@jlebon
Copy link
Member

jlebon commented Apr 20, 2020

Right yeah. Definitely not going to argue that a compiled language is better for writing complex logic than shell. :) That said, just the fact that the shell script runs directly on the host now vs e.g. via SSH is already a massive improvement saving us a lot of pain (such as the hungry quotes issue, rebooting, more precise error-handling, etc...). For the majority of tests, the shell script business logic is pretty simple and have understood patterns at this point.

So maybe we instead have a mix of shell-based tests and compiled tests and helpers? To me a lot of the value of coreos/coreos-assembler#1215 (apart from making it easier to fix the binding problem) is that it's super easy to add a test with a few lines of shell script and just throw that up into the host and run it. If we add compiled binaries into the mix, then (1) it's harder to write tests (that's a bit what we're getting away from by moving tests outside of Golang in kola, right?), and (2) we're introducing a requirement on the test infrastructure. And maybe that's fine here (we're already compiling C and Rust code anyway), but as a general pattern across the projects, it makes it harder to re-use upstream tests downstream.

(One interesting idea on that subject is if the shell script compiled the bits it needs directly on the host system in a container. That way, it's still entirely self-contained.)

@cgwalters
Copy link
Member Author

If we add compiled binaries into the mix, then (1) it's harder to write tests (that's a bit what we're getting away from by moving tests outside of Golang in kola, right?),

Yes and no. A number of the kola Go tests were just "shell in ssh commands in Go" - clearly the exttest model is better for that, particularly when the test is < 10 lines.

But I also want to support writing tests in a real language (Rust or Go or whatever). Part of this PR is also exercising the OSTree shared library API (via Rust since it's convenient), which we can only do indirectly from shell.

And yes, there is more infrastructure required there. It goes right back to having an ostree-tests package (probably upstream, make install-tests).

(One interesting idea on that subject is if the shell script compiled the bits it needs directly on the host system in a container. That way, it's still entirely self-contained.)

Yeah we should support that too (make install-tests in a Dockerfile). But...we need at least some of our tests to support testing the container infrastructure itself. Plus in many cases it's just simpler to inject a static binary. And finally, if we start pulling external containers in our CI system that makes those tests unnecessarily require the Internet and an external registry. (Now, at least for qemu I'd like kola to support binding in containers from the host via 9p/virtiofs which would address that)

@cgwalters
Copy link
Member Author

OK still working on a few things here. Notes so far:

I created https://github.com/cgwalters/with-procspawn-tempdir/ so we can drop a whole lot of tmp_dir usage in the test functions.

I also was just searching for a way to do "standard Rust" like #[test] attribute and discovered
https://github.com/dtolnay/linkme
which I think will work...going to try that soon.

@cgwalters
Copy link
Member Author

I created https://github.com/cgwalters/with-procspawn-tempdir/ so we can drop a whole lot of tmp_dir usage in the test functions.

Is now in use, though see also mitsuhiko/procspawn#33

https://github.com/dtolnay/linkme

Works great! Now with 100% less "function to return list of test functions".

@cgwalters
Copy link
Member Author

OK rebasing and lifting WIP on this. We do need a bit more thought around how this all works. One random thing I'm considering now is supporting /usr/lib/coreos-assembler/tests (which would be strongly related to coreos/coreos-assembler#1273 ) - so the workflow would be make build-tests && sudo make install-tests and we'd drop them in there, so that a further cosa kola run would pick them up.

This workflow would also be a step towards more obviously supporting an ostree-tests RPM and/or container image.

@cgwalters cgwalters changed the title WIP: Add new Rust-based tests Add new Rust-based tests May 8, 2020
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request May 11, 2020
In continuing work on external tests, passing `-E` gets awkward
for a few reasons.  First, it ties the tests too heavily to the
source directory.

This supports a model like https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests
and Debian autopkg test etc. where the tests are installed.

This ensures a clean separation from the source directory,
which also helps us in e.g. supporting built binaries as is done
in ostreedev/ostree#2048

For example, one thing we can do after this is extend the
coreos-assembler container with the results of `make install-tests`
from relevant git repositories (or make RPMs of them).
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request May 11, 2020
In continuing work on external tests, passing `-E` gets awkward
for a few reasons.  First, it ties the tests too heavily to the
source directory.

This supports a model like https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests
and Debian autopkg test etc. where the tests are installed.

This ensures a clean separation from the source directory,
which also helps us in e.g. supporting built binaries as is done
in ostreedev/ostree#2048

For example, one thing we can do after this is extend the
coreos-assembler container with the results of `make install-tests`
from relevant git repositories (or make RPMs of them).
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request May 11, 2020
In continuing work on external tests, passing `-E` gets awkward
for a few reasons.  First, it ties the tests too heavily to the
source directory.

This supports a model like https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests
and Debian autopkg test etc. where the tests are installed.

This ensures a clean separation from the source directory,
which also helps us in e.g. supporting built binaries as is done
in ostreedev/ostree#2048

For example, one thing we can do after this is extend the
coreos-assembler container with the results of `make install-tests`
from relevant git repositories (or make RPMs of them).
@cgwalters
Copy link
Member Author

Will adapt this after coreos/coreos-assembler#1441 lands.

openshift-merge-robot pushed a commit to coreos/coreos-assembler that referenced this pull request May 13, 2020
In continuing work on external tests, passing `-E` gets awkward
for a few reasons.  First, it ties the tests too heavily to the
source directory.

This supports a model like https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests
and Debian autopkg test etc. where the tests are installed.

This ensures a clean separation from the source directory,
which also helps us in e.g. supporting built binaries as is done
in ostreedev/ostree#2048

For example, one thing we can do after this is extend the
coreos-assembler container with the results of `make install-tests`
from relevant git repositories (or make RPMs of them).
cgwalters added a commit to cgwalters/ostree that referenced this pull request May 14, 2020
This builds on
coreos/coreos-assembler#1441
to install our tests rather than running them from the source
directory.  This model will more cleanly allow us to ship
our tests along with a test container or elsewhere, separate
from the source directory.

Also prep for ostreedev#2048
cgwalters added a commit to cgwalters/ostree that referenced this pull request May 15, 2020
This builds on
coreos/coreos-assembler#1441
to install our tests rather than running them from the source
directory.  This model will more cleanly allow us to ship
our tests along with a test container or elsewhere, separate
from the source directory.

Also prep for ostreedev#2048
cgwalters added a commit to cgwalters/ostree that referenced this pull request May 15, 2020
This builds on
coreos/coreos-assembler#1441
to install our tests rather than running them from the source
directory.  This model will more cleanly allow us to ship
our tests along with a test container or elsewhere, separate
from the source directory.

Also prep for ostreedev#2048
cgwalters added a commit to cgwalters/ostree that referenced this pull request May 15, 2020
This builds on
coreos/coreos-assembler#1441
to install our tests rather than running them from the source
directory.  This model will more cleanly allow us to ship
our tests along with a test container or elsewhere, separate
from the source directory.

Also prep for ostreedev#2048
@cgwalters cgwalters force-pushed the rust-cmdspec-tests branch 4 times, most recently from 13215cc to 7f0caa0 Compare May 18, 2020 19:52
@cgwalters
Copy link
Member Author

Cool, all working now:

May 18 20:14:30.491973 kola-runext-ostree-test[1199]: test test_pull_basicauth ... ok
May 18 20:14:30.518625 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.520196 kola-runext-ostree-test[1199]: test test_extensions     ... ok
May 18 20:14:30.542589 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.558625 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.575695 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.579275 kola-runext-ostree-test[1199]: test test_mtime          ... ok
May 18 20:14:30.605621 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.627664 kernel: xfs filesystem being remounted at /sysroot supports timestamps until 2038 (0x7fffffff)
May 18 20:14:30.630320 kola-runext-ostree-test[1199]: test test_nofifo         ... ok
May 18 20:14:30.641894 kola-runext-ostree-test[1199]: test test_basic          ... ok
May 18 20:14:30.644493 kola-runext-ostree-test[1199]: test test_sysroot_ro     ... ok
May 18 20:14:30.644619 kola-runext-ostree-test[1199]: test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

@cgwalters
Copy link
Member Author

(And OK there's only so many times I can see that timestamp warning so containers/bubblewrap#377 )

There's a lot going on here.  First, this is intended to run
nicely as part of the new [cosa/kola ext-tests](coreos/coreos-assembler#1252).

With Rust we can get one big static binary that we can upload,
and include a webserver as part of the binary.  This way we don't
need to do the hack of running a container with Python or whatever.

Now, what's even better about Rust for this is that it has macros,
and specifically we are using [commandspec](https://github.com/tcr/commandspec/)
which allows us to "inline" shell script.  I think the macros
could be even better, but this shows how we can intermix
pure Rust code along with using shell safely enough.

We're using my fork of commandspec because the upstream hasn't
merged [a few PRs](https://github.com/tcr/commandspec/pulls?q=is%3Apr+author%3Acgwalters+).

This model is intended to replace *both* some of our
`make check` tests as well.

Oh, and this takes the obvious step of using the Rust OSTree bindings
as part of our tests.  Currently the "commandspec tests" and "API tests"
are separate, but nothing stops us from intermixing them if we wanted.

I haven't yet tried to write destructive tests with this but
I think it will go well.
@cgwalters
Copy link
Member Author

OK rebased 🏄 on top of tests/kolainst.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool. Definitely interested to try this out! Looks sane at a high-level, just some comments below, though if you prefer we can address those as follow-ups.

To double-check, we still retain the ability to write simple tests in shell script like this if we want, right?

Comment on lines +91 to +95
osroot = osroot.to_str(),
serverrepo = serverrepo.to_str(),
baseuri = baseuri.to_string(),
unauthuri = unauthuri.to_string(),
authuri = authuri.to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish Rust had Python-style string interpolation.

Oh hey, looks like there's a crate for that!
https://crates.io/crates/ifmt

Want to try that out? Should make it nicer to hack on. (Not a blocker of course.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more thinking of adding this to the commandspec crate (which we are forking right now because the maintainer is unresponsive). Though the implementation of that could indeed be ifmt perhaps?

top=$(git rev-parse --show-toplevel)
cd ${top}
make
cosa build-fast
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like cosa build-fast should print a banner or something to make it clear what the caveats are. Or if we don't want to put it there, then at least in higher-level hacking scripts like this one which use it. Otherwise, I can definitely see OSTree developers in the future who might not be familiar with the underlying tools banging their head trying to figure out why e.g. their initramfs-related change isn't being picked up.

Anyway, don't have to address this right away.

@cgwalters
Copy link
Member Author

To double-check, we still retain the ability to write simple tests in shell script like this if we want, right?

Yes, of course. More generally the kola ext model just runs executables.

Also, I do think that a number of these tests mostly are shell, they're just wrapped in a Rust function. We could make that even easier to do.

Anyways I have a bunch of pending work on top of this to try to add support for destructive tests. But it would be nice to get this merged before that too.

@jlebon
Copy link
Member

jlebon commented Jun 2, 2020

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 2598612 into ostreedev:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants