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

feat(testing): add basic section-based test declaration support #65

Merged
merged 8 commits into from
Feb 8, 2020

Conversation

mystor
Copy link
Collaborator

@mystor mystor commented Feb 7, 2020

This is an outline of a potential section-based approach for declaring tests which supports running tests from across the dependency graph with a single test runner.

May supercede #59

Example Output:

 [INFO] run tests
 [INFO] | test, test.name="basic_alloc", test.module="mycelium_kernel"
 [INFO] | |- mycelium_kernel: vec=[], vec.addr=0x8
 [INFO] | |- mycelium_kernel: vec=[5], vec.addr=0x3fc058
 [INFO] | |- mycelium_kernel: vec=[5, 10], vec.addr=0x3fc048
 [INFO] | test, test.name="wasm_hello_world", test.module="mycelium_kernel"
[TRACE] | | invoke_index, index=0, args=RuntimeArgs([I32(1), I32(0), I32(1), I32(20)])
 [INFO] | | | fd_write, fd=1, iovs=0, iovs_len=1, nwritten=20
 [INFO] | | | |- mycelium_kernel::wasm::wasi: fd_write, buf_slice=[104, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, 10], buf_str=Ok("hello world\n")
 [INFO] | test, test.name="it_works", test.module="mycelium_util::testing"
 [INFO] | |- mycelium_util::testing: I'm running in a test!
 [WARN] |- mycelium_kernel: 3 passed | 0 failed

@mystor mystor requested a review from hawkw February 7, 2020 03:43
// Introduce an anonymous const to avoid name conflicts. The `#[used]`
// will prevent the symbol from being dropped, and `link_section` will
// make it visible.
const _: () = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, we can't put this behind a #[cfg(test)], as that cfg flag won't be visible in dependencies. We may need to use RUSTFLAGS="--cfg mycelium_test" (or something like that) to turn on test code for the whole dependency tree.

Copy link
Owner

Choose a reason for hiding this comment

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

that makes sense, i would really like to avoid linking these into non-test builds eventually. this is fine for now...

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this is awesome! i left notes on a few things but it will be very cool to get this merged.

/// Get a list of test objects.
pub fn all_tests() -> &'static [Test] {
unsafe {
// FIXME: These should probably be `&raw const __start_*`.
Copy link
Owner

Choose a reason for hiding this comment

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

is &raw new proposed syntax? i would love a link to this as I've never seen it before!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! It's a way to do field indexing without requiring the object you're taking a reference to to be valid memory. (rfc: rust-lang/rfcs#2582, tracking issue: rust-lang/rust#64490)

use core::slice;

/// Internal definition type used for a test.
pub struct Test {
Copy link
Owner

Choose a reason for hiding this comment

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

nit/tioli: since this has all pub fields, should there maybe be a private _p: () or something to stop it from being constructed from anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has to be constructed from within the macro, which is why it has all-pub fields, so I can't add a _p: () or the decl_test! macro won't work.

It's technically harmless to construct it anywhere you like, it won't do anything unless it's in the correct section.

Copy link
Owner

Choose a reason for hiding this comment

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

ah, nvm, i missed that! seems like it could have a const-fn ctor but that might just be boilerplate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that might just end up being boilerplate. I'm inclined to just leave it as-is.

use core::mem;
use core::slice;

/// Internal definition type used for a test.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: the test runner in the kernel consumes this, so i am not sure if i would call it "internal" :P

// Introduce an anonymous const to avoid name conflicts. The `#[used]`
// will prevent the symbol from being dropped, and `link_section` will
// make it visible.
const _: () = {
Copy link
Owner

Choose a reason for hiding this comment

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

that makes sense, i would really like to avoid linking these into non-test builds eventually. this is fine for now...

util/src/testing.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Comment on lines 175 to 180
pub(crate) fn qemu_exit(exit_code: QemuExitCode) {
let code = exit_code as u32;
unsafe {
asm!("out 0xf4, eax" :: "{eax}"(code) :: "intel","volatile");
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

can we stick a

#[cfg(test)]
qemu_exit(QemuExitCode::Failed);

at the end of arch::x86_64::oops, before the loop? that way, if a test panics/faults, we can report failure rather than hanging. ideally, we would be able to track which test failed, so that panics can be a nice way to fail tests rather than presenting less information, but it would be nice if panics/faults didn't make the CI build just hang

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I was intending to do that, but forgot to do it before pushing. I'll add that.

Comment on lines +40 to +41
- name: install qemu
run: sudo apt-get update && sudo apt-get install qemu
Copy link
Owner

Choose a reason for hiding this comment

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

it would be nice if we could figure out a way to use the github actions cache thingy so we don't have to install qemu over and over...not a blocket though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't even know that was a thing, so I'm not sure I can help with that :-P
If you have some documentation for how the cache thing works to point me to, let me know.

Copy link
Owner

Choose a reason for hiding this comment

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

we can figure this out later!

command: dev-env
- name: install qemu
run: sudo apt-get update && sudo apt-get install qemu
- name: run tests
Copy link
Owner

Choose a reason for hiding this comment

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

would be good to have a step that runs any "regular" (i.e. not in qemu) rust tests in crates as well. i think we define some now as of #64, so it would be good for them to run on CI as well.

if this was a separate job, it could run in parallel with the qemu tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we should probably do that. We'll probably want to make sure to exclude the root mycelium_kernel crate, though.

Copy link
Owner

Choose a reason for hiding this comment

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

we might have to just cargo test -p $FOO for every other crate...which is fine, it's just a little flaky since it needs to be manually updated as we add more crates...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up just using more #[cfg(...)] attributes to stub out a dummy fn main() for the mycelium_kernel crate when we're running on the host platform.

@hawkw
Copy link
Owner

hawkw commented Feb 8, 2020

clippy seems broken for some reason and i don't know why? but otherwise, this is awesome

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

shipit!

@mystor mystor merged commit 7cf668d into master Feb 8, 2020
@mystor mystor deleted the nika/section-testing branch February 8, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants