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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cargo/config
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ runner = "bootimage runner"
dev-env = "install cargo-xbuild bootimage"
run-x64 = "xrun --target=x86_64-mycelium.json"
debug-x64 = "xrun --target=x86_64-mycelium.json -- -gdb tcp::1234 -S"
test-x64 = "xtest --target=x86_64-mycelium.json"
21 changes: 19 additions & 2 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,27 @@ jobs:
with:
command: bootimage
args: --target=x86_64-mycelium.json
- name: Test

test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- name: install rust toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
components: rust-src, llvm-tools-preview
- name: install dev env
uses: actions-rs/[email protected]
with:
command: dev-env
- name: install qemu
run: sudo apt-get update && sudo apt-get install qemu
Comment on lines +40 to +41
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!

- 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.

uses: actions-rs/[email protected]
with:
command: test
command: test-x64

clippy:
runs-on: ubuntu-latest
Expand Down
12 changes: 12 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@ edition = "2018"

[lib]
name = "mycelium_kernel"
harness = false

[[bin]]
name = "mycelium_kernel"
path = "src/main.rs"
test = false

[dependencies]
hal-core = { path = "hal-core" }
mycelium-alloc = { path = "alloc" }
mycelium-util = { path = "util" }
tracing = { version = "0.1", default_features = false }

[target.'cfg(target_arch = "x86_64")'.dependencies]
Expand All @@ -35,6 +42,11 @@ wat = "1.0"

[package.metadata.bootimage]
default-target = "x86_64-mycelium.json"
test-success-exit-code = 33 # (0x10 << 1) | 1
test-args = [
"-device", "isa-debug-exit,iobase=0xf4,iosize=0x04",
"-serial", "stdio", "-display", "none"
]

[package.metadata.target.'cfg(target_arch = "x86_64")'.cargo-xbuild]
memcpy = true
Expand Down
19 changes: 19 additions & 0 deletions src/arch/x86_64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,22 @@ pub fn oops(cause: &dyn core::fmt::Display) -> ! {
}
}
}

#[cfg(test)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[repr(u32)]
pub(crate) enum QemuExitCode {
Success = 0x10,
Failed = 0x11,
}

/// Exit using `isa-debug-exit`, for use in tests.
///
/// NOTE: This is a temporary mechanism until we get proper shutdown implemented.
#[cfg(test)]
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.

93 changes: 77 additions & 16 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#![cfg_attr(test, no_main)]
#![cfg_attr(target_os = "none", no_std)]
#![cfg_attr(target_os = "none", feature(alloc_error_handler))]
#![cfg_attr(target_os = "none", feature(panic_info_message, track_caller))]
#![feature(asm)]
extern crate alloc;

Expand Down Expand Up @@ -62,9 +64,42 @@ pub fn kernel_main(bootinfo: &impl BootInfo) -> ! {

arch::interrupt::init::<arch::InterruptHandlers>();

#[cfg(test)]
{
let span = tracing::info_span!("alloc test");
let span = tracing::info_span!("run tests");
let _enter = span.enter();

let mut passed = 0;
let mut failed = 0;
for test in mycelium_util::testing::all_tests() {
let span = tracing::info_span!("test", test.name, test.module);
let _enter = span.enter();

if (test.run)() {
passed += 1;
} else {
failed += 1;
}
}

tracing::warn!("{} passed | {} failed", passed, failed);
if failed == 0 {
arch::qemu_exit(arch::QemuExitCode::Success);
} else {
arch::qemu_exit(arch::QemuExitCode::Failed);
}
}

// if this function returns we would boot loop. Hang, instead, so the debug
// output can be read.
//
// eventually we'll call into a kernel main loop here...
#[allow(clippy::empty_loop)]
loop {}
}

mycelium_util::decl_test! {
fn basic_alloc() {
// Let's allocate something, for funsies
let mut v = Vec::new();
tracing::info!(vec = ?v, vec.addr = ?v.as_ptr());
Expand All @@ -75,23 +110,12 @@ pub fn kernel_main(bootinfo: &impl BootInfo) -> ! {
assert_eq!(v.pop(), Some(10));
assert_eq!(v.pop(), Some(5));
}
}

{
let span = tracing::info_span!("wasm test");
let _enter = span.enter();

match wasm::run_wasm(HELLOWORLD_WASM) {
Ok(()) => tracing::info!("wasm test Ok!"),
Err(err) => tracing::error!(?err, "wasm test Err"),
}
mycelium_util::decl_test! {
fn wasm_hello_world() -> Result<(), wasmi::Error> {
wasm::run_wasm(HELLOWORLD_WASM)
}

// if this function returns we would boot loop. Hang, instead, so the debug
// output can be read.
//
// eventually we'll call into a kernel main loop here...
#[allow(clippy::empty_loop)]
loop {}
}

#[global_allocator]
Expand All @@ -103,3 +127,40 @@ pub static GLOBAL: mycelium_alloc::Alloc = mycelium_alloc::Alloc;
fn alloc_error(layout: core::alloc::Layout) -> ! {
panic!("alloc error: {:?}", layout);
}

#[cfg(target_os = "none")]
#[panic_handler]
#[cold]
fn panic(panic: &core::panic::PanicInfo) -> ! {
hawkw marked this conversation as resolved.
Show resolved Hide resolved
use core::fmt;

struct PrettyPanic<'a>(&'a core::panic::PanicInfo<'a>);
impl<'a> fmt::Display for PrettyPanic<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let message = self.0.message();
let location = self.0.location();
let caller = core::panic::Location::caller();
if let Some(message) = message {
writeln!(f, " mycelium panicked: {}", message)?;
if let Some(loc) = location {
writeln!(f, " at: {}:{}:{}", loc.file(), loc.line(), loc.column(),)?;
}
} else {
writeln!(f, " mycelium panicked: {}", self.0)?;
}
writeln!(
f,
" in: {}:{}:{}",
caller.file(),
caller.line(),
caller.column()
)?;
Ok(())
}
}

let caller = core::panic::Location::caller();
tracing::error!(%panic, ?caller);
let pp = PrettyPanic(panic);
arch::oops(&pp)
}
45 changes: 2 additions & 43 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,47 +3,6 @@
#![cfg_attr(target_os = "none", feature(alloc_error_handler))]
#![cfg_attr(target_os = "none", feature(asm))]
#![cfg_attr(target_os = "none", feature(panic_info_message, track_caller))]
use mycelium_kernel::arch;

#[cfg(target_os = "none")]
#[panic_handler]
#[cold]
fn panic(panic: &core::panic::PanicInfo) -> ! {
use core::fmt;

struct PrettyPanic<'a>(&'a core::panic::PanicInfo<'a>);
impl<'a> fmt::Display for PrettyPanic<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let message = self.0.message();
let location = self.0.location();
let caller = core::panic::Location::caller();
if let Some(message) = message {
writeln!(f, " mycelium panicked: {}", message)?;
if let Some(loc) = location {
writeln!(f, " at: {}:{}:{}", loc.file(), loc.line(), loc.column(),)?;
}
} else {
writeln!(f, " mycelium panicked: {}", self.0)?;
}
writeln!(
f,
" in: {}:{}:{}",
caller.file(),
caller.line(),
caller.column()
)?;
Ok(())
}
}

let caller = core::panic::Location::caller();
tracing::error!(%panic, ?caller);
let pp = PrettyPanic(panic);
arch::oops(&pp)
}

fn main() {
unsafe {
core::hint::unreachable_unchecked();
}
}
// Force linking to the `mycelium_kernel` lib.
use mycelium_kernel;
1 change: 1 addition & 0 deletions util/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ edition = "2018"
alloc = []

[dependencies]
tracing = { version = "0.1", default_features = false }
loom = { version = "0.2.14", optional = true }

[dev-dependencies]
Expand Down
1 change: 1 addition & 0 deletions util/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ pub mod cell;
pub mod error;
pub mod io;
pub mod sync;
pub mod testing;
95 changes: 95 additions & 0 deletions util/src/testing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use core::fmt;
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

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.

pub module: &'static str,
pub name: &'static str,
pub run: fn() -> bool,
hawkw marked this conversation as resolved.
Show resolved Hide resolved
}

/// Type which may be used as a test return type.
pub trait TestResult {
/// Report any errors to `tracing`, then returns either `true` for a
/// success, or `false` for a failure.
fn report(self) -> bool;
}

impl TestResult for () {
fn report(self) -> bool {
true
}
}

impl<T: fmt::Debug> TestResult for Result<(), T> {
fn report(self) -> bool {
match self {
Ok(_) => true,
Err(err) => {
tracing::error!("FAIL {:?}", err);
false
}
}
}
}

/// Declare a new test, sort-of like the `#[test]` attribute.
// FIXME: Declare a `#[test]` custom attribute instead?
#[macro_export]
macro_rules! decl_test {
(fn $name:ident $($t:tt)*) => {
fn $name $($t)*

// 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...

#[used]
#[link_section = "MyceliumTests"]
static TEST: $crate::testing::Test = $crate::testing::Test {
module: module_path!(),
name: stringify!($name),
run: || $crate::testing::TestResult::report($name()),
};
};
}
}

// These symbols are auto-generated by lld (and similar linkers) for data
// `link_section` sections, and are located at the beginning and end of the
// section.
//
// The memory region between the two symbols will contain an array of `Test`
// instances.
extern "C" {
static __start_MyceliumTests: ();
static __stop_MyceliumTests: ();
}

/// 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)

let start: *const () = &__start_MyceliumTests;
let stop: *const () = &__stop_MyceliumTests;

let len_bytes = (stop as usize) - (start as usize);
let len = len_bytes / mem::size_of::<Test>();
assert!(len_bytes % mem::size_of::<Test>() == 0,
"Section should contain a whole number of `Test`s");

if len > 0 {
slice::from_raw_parts(start as *const Test, len)
} else {
&[]
}
}
}

decl_test! {
fn it_works() -> Result<(), ()> {
tracing::info!("I'm running in a test!");
Ok(())
}
}