Explicitly panic (so test actually fails) if we cannot run it

[bleggett]

eprintln doesn't actually fail the test in local cargo test runs - resulting in the test "silently" being skipped, which is not great if it would actually fail in the required environment.

[howardjohn]

This was intended; in CI it panics but for local dev it does not so you don't have to run these tests. Basically equivalent to t.Skip.

Maybe t.Skip is not the best path here though? WDYT? The intent was to make cargo test actually just work without requiring hoops

[bleggett]

This was intended; in CI it panics but for local dev it does not so you don't have to run these tests. Basically equivalent to t.Skip.

Maybe t.Skip is not the best path here though? WDYT? The intent was to make cargo test actually just work without requiring hoops

I don't think it's great local UX - without the panic, the following is the result when these tests (which are actually failing for me locally in a branch when run as root) are run "normally":

 » cargo test test_process_add                                                                                                               
    Blocking waiting for file lock on build directory
   Compiling ztunnel v0.0.0 (/home/bleggett/Source/ztunnel)
    Finished test [unoptimized + debuginfo] target(s) in 29.04s
     Running unittests src/lib.rs (out/rust/debug/deps/ztunnel-7fd42b2982b9b6f4)

running 2 tests
test inpod::workloadmanager::tests::test_process_add_and_del ... ok
test inpod::workloadmanager::tests::test_process_add ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 156 filtered out; finished in 0.00s

They fail as root, because they're actually running as root. But it's not clear at all from the output here that they aren't being run at all when I'm not running as root.

I'd rather them fail explicitly if they cannot be run.

[howardjohn]

Yeah it will at least never pass CI though. If you go test ./... in Istio you are skipping tons of tests too (all the integration ones). Local test is not necessarily fully complete.

If we do this should we at least put CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=sudo in the config.toml so people don't have to specify it ? And I suppose we can enumerate a variety of targets (wish you didn't need this but... )

[bleggett]

Yeah it will at least never pass CI though. If you go test ./... in Istio you are skipping tons of tests too (all the integration ones). Local test is not necessarily fully complete.

That's fine - if it says skipped or ignored that's one thing - if it says passed when it didn't actually run, that's really confusing.

Apparently cargo test has no runtime-conditional skipping, so it's basically panic or nothing.

If we do this should we at least put CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=sudo in the config.toml so people don't have to specify it ? And I suppose we can enumerate a variety of targets (wish you didn't need this but... )

We are already setting this with the make test-root target.

[howardjohn]

Yeah it will at least never pass CI though. If you go test ./... in Istio you are skipping tons of tests too (all the integration ones). Local test is not necessarily fully complete.

That's fine - if it says skipped or ignored that's one thing - if it says passed when it didn't actually run, that's really confusing.

Yeah, rust doesn't support this :-(

If we do this should we at least put CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER=sudo in the config.toml so people don't have to specify it ? And I suppose we can enumerate a variety of targets (wish you didn't need this but... )

We are already setting this with the make test-root target.

Sure, but I think its a pretty good property to run cargo test (or go test) on a project and have it just work instead of remembering obscure project-specific makefile commands. plus, its needed for interactive use (maybe i want some flags, specific tests, etc). While I personally can just set CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER since I work on ztunnel a lot, its probably a bit of friction for the occasionally contributor

[howardjohn]

or at the very least maybe we can explain in the error how to set CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER/equivilent

[bleggett]

I also note that the sudo prompt you get with the current makefile export of CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER requires a password, and since we are defaulting to BUILD_WITH_CONTAINER=1 - it seems to be asking for the root password of the in-container user, not the local user, which is probably confusing to most people.

Do we not add the build-tools user to the container-local local wheel group?

I'll see if I can fix that, I'm a bit hesitant to auto-sudo test runs without that.

[howardjohn]

Ah didn't know that, I have passwordless sudo on my host and don't use the build container. Good catch

[bleggett]

Aight I settled on if TEST_MODE=root && BUILD_WITH_CONTAINER=1

1. We tell you this means we have to run the build container as privileged (this is what the vscode devcontainer stuff already does, actually)
2. We override the docker args to do that.
3. If you don't want that (or can't in your env), then we tell you to run with BUILD_WITH_CONTAINER=0

I still think we shouldn't have unit tests that run as root and would rather mock OS deps or move these to integ tests, they by definition aren't unit tests if they require root privs IMO - but that's another discussion.

[bleggett]

#962 just drops the root req, so this is no longer needed