search

bytecodealliance / lucet

Lucet, the Sandboxing WebAssembly Compiler.
Apache License 2.0
4.06k stars 165 forks source link

we should test lucet-runtime! #560

Closed iximeow closed 4 years ago

iximeow commented 4 years ago

~I think I found a typo in some of our yaml programming. CRATES_NOT_TESTED included lucet-runtime, but that's where the runtime tests are instantiated and run. Instead, we should ignore lucet-runtime-tests for testing (though not strictly necessary - there are no tests in there anyway).~

Edit: not a typo, this is very much intended: we don't want to run tests for UffdRegion on github actions, because the Linux there doesn't support that kernel API.

I think this is why https://github.com/bytecodealliance/lucet/pull/554 got as far as the lucet-objdump smoke test before failing, since from a quick eyeball the tests that are run either test details of Cranelift state, module translation state, or use mock modules. lucet-runtime is the only place we actually load up a real honest-to-goodness DlModule, which I believe should fail on #554 for to-be-determined weirdness between lld and lucetc's choices of relocation.

Independently testing that lucet-runtime really was not being tested by waiting on CI for this very cool new test I wrote.

iximeow commented 4 years ago

cc @pchickey because I think you'd best know if I've misunderstood the github action, or if this is a :heavy_check_mark: change.

iximeow commented 4 years ago

~edit: the very cool test doesn't fail github actions on main, whoops. Circle and Mac actions still run lucet-runtime tests so we got coverage on commits that got a :heavy_check_mark: by CI.~ Working as intended, see OP

iximeow commented 4 years ago

@fst-crenshaw pointed out that this was originally added (with good discussion!) in https://github.com/bytecodealliance/lucet/pull/528 - this is me getting ahead of myself on a Friday afternoon, sorry :smile_cat: