Unless they're independently justifiable, two ways of doing the same thing is generally worse.
To me, if the purpose of this is to be conditional on a runtime condition, code is the way to do it -- especially if it's done via unwinding because I think it's easier to understand the first line of the test being a call to ignore_test_if_not_root(); than anything about predicates inside attributes.
I could, perhaps, see an unconditional#[skip] attribute as being a reasonable thing to also have. That's distinct to me as it's knowable with running anything, and maybe it could be reasonable to cfg_attr it instead of "running" a cfg! check. I've certainly seen many unit test runners with an Ignore or similar attribute.
Personally, I'm not too keen on the code version because, AFAIK, the test crate is perma-unstable. If I can't use this on stable toolchains, I have to keep my testsuite as nightly-only or have bogus passing tests (because test failure means it's hard to find actual failures). Neither of those sounds very good to me.
Is there a path to getting the test crate to become stable?
The point is that these conditions ("is the currently running kernel new enough", "are we running with a usable network") are not knowable at build time, so cfg_attr doesn't work (if it did, I'd be satisfied just compiling the test out completely).
So after some time and thought with some other community discussion, I think that it would be far better to instead have a dedicated exit code which indicates "this test will be skipped". This would block stabilization on impl Termination which at least has some traction. If this is suitable, we then just have the bikeshedding of:
which exit code to use
prior art being 77 from autoconf (IMO, stronger) and 125 from git bisect (IMO, weaker)
how to represent skip reasons
Is there any reason to prefer a skip_if predicate (not composable) or panicking (breaks encapsulation and confounds/complicates catch_unwind-using tests) given the rationales in that blog post? Of course, I'll transpose this update into the new RFC text before submitting it formally.
Another annoying thing about using impl Termination is that you have to change the signature of the test functions whenever you want to add a skip (or remove a skip, though that's technically optional).
I really dislike the idea of a "magic" integer that marks a test as skipped (though if this is the route chosen, I might suggest 0x534b4950 as the magic value, which is 'SKIP'). At the very least Rust should provide a named const for this, but then you need to stabilize the test crate.
There's also a lot more boilerplate required with this approach than I'd like to have to implement. Skipping a test should be simple and easy to add and remove.
I'd greatly prefer either a macro (e.g., skip!("reason for skipping"), panic_any!(test::Skip("reason for skipping"), or whatever) or a function to facilitate test skipping (e.g., test::skip("reason for skipping")). Ideally these would be diverging functions so you don't have to call return. If test stabilization is a major concern, could you add them to the test preamble so you don't have to stabilize test?
This is in reference to the panic_any!(skip_type) mechanism, not the attribute. It's that if you have some test which does catch_unwind, it needs to handle these panics which now may be coming through "just" to skip the test.
This is no different than adding an error condition to a function: you're going to go from T to Result<T, ErrType>.
Alas, with executable tests, we need exit_code from the process to communicate this informationā¦there, we only have 0..=127 which is portable .
In my blog post, I cover the case where one might want to call some routine which wants to skip, but you know of a way to recover from it (say Vulkan is not available, but OpenGL is and you can flip the renderer over). In this case, just dealing with normal types is far nicer than catch_unwind.
AFAICT, test will never be stable (though it may also move out of the compiler). What is "the test preamble"?
I think this concern is focused on an edge case that 99.9% of users/tests would never hit. I've used test skipping in Objective-C quite a bit (which uses exceptions to skip tests) and I have never put a call to XCTSkip anywhere close to (or inside of) a try block. Every single time I've skipped a test it's been something like:
- (void)testFoo {
XCTSkip(@"TODO: fix this test so it is no longer flaky");
// ...code...
}
- (void)testBar {
if (@available(iOS 14.5, *)) {
} else {
XCTSkip("This test depends on an API that requires iOS 14.5+");
}
}
I think it's extremely likely that someone will be skipping a test inside of a catch_unwind block.
True, but my point is that many test skips are temporary and it causes extra code churn. It's not a huge deal, but it is annoying.
Are subprocess tests a thing in Rust? If so that's news to me.
My counter argument would be that the code is designed wrong Of course that's an opinionated statement but IMO skip calls should be placed in the test itself, not in subroutines.
I made up the term "test preamble" but the basic idea is that tests get access to a slightly broader preamble than non-test code. This could be implemented in a few ways, but one way would be to have the #[test] macro inject a use declaration at the top of the function. That is:
#[test]
fn test_foo() {
}
gets transformed into something like:
fn test_foo() {
use ::test::skip_test;
}
This way the test crate can still be unstable (since bultin macros can expand to code that accesses unstable APIs) but users could still have access to skip_test().
Okā¦so then why is the attribute not superior here if we're not interested in something like "deeply nested skip detection"? Why are we adding a mechanism that only works through panicking which is otherwise a "something very bad has gone wrong" codepath?
Temporary in what sense? Over the history of the project? I don't think so at all. Unless a project is never using new code and can actually outwait the EOL schedules (nevermind how long such things get used past their EOL dates) of the dependent infrastructure, there are skip mechanisms that will always be needed I think. Especially in the case I have today in Rust: running as root breaks the test and I'd like to skip it. I don't think anyone is proposing that root be able to make files it cannot read (without other platform-specific hoops to jump through).
Then I find the attribute strictly superior in this case. About the only thing that a panic!-based mechanism had was that it can "escape" from whatever scope to allow for refactoring.
That's not how the testing mechanism works right now at all; the body of the function is not touched. In any case, I don't think injecting new symbols and potentially shadowing other skip_test imports is a viable path (even if I think the chance of such code existing today is low).
.
Of course that's an opinionated statement but IMO skip calls should be placed in the test itself, not in subroutines.