Copy link

yanganto commented 10 days ago •

There are three issues mixed here,

• one is to ignore the test case on runtime,
• another is to ignore or run the test case with conditions,
• the last one is the syntax change.

It may be good to separate these issues into different RFCs and figure out how to do.

About runtime ignoring
We need to enhance because there is a lot of voice that wants this kind of feature:

About the issue to ignore or test with conditions
there is already voice to leave the condition things into frameworks and keep rust simple here. Such that test-with is here and try to solve, and willing to change if there is any possible to ignore in runtime.

About the syntax
We already have a syntax to handle the ignored message or reason #[ignore = "not yet implemented"], and also already have a guild in rust reference.
Therefore, it is good to keep the syntax, because people are used to writing like this, and there is no good reason to change syntax. Besides, the original way is simple and clear.

Forget the implementation at first, ignore!() macro may have a clear syntax to let conditions ignoring test cases and runtime ignoring can be straightforward, we do not force people to check other functions, and if the needed, people still can make the condition from the results of a function.

#[test]
fn test_case(){
  if env::var("SomeVar").is_none() {
     ignore!("something we need is not here")
  }
  keep_test();
}

It is really happy to see you here.

mina86 commented 10 days ago

• one is to ignore the test case on runtime,

• another is to ignore or run the test case with conditions,

Those are the same question though?

Regardless, this cannot be separated because answers to earlier questions influence subsequent decisions. It’s not possible to decide on syntax if it’s not decided whether ignoring --ignored and --include-ignored flags is acceptable. There are three main questions here:

1. Whether we want to have this at all.
2. Whether we care about --include-ignored and --ignored.
3. What syntax to use.

Regarding first question, it of course can be (and has been) argued that this feature should be left to test frameworks however the same could be said about about #[ignore] and since we have that annotation it’s strong suggestion that being able to ignore tests at runtime should be supported as well. Majority of testing frameworks which support skipping tests allow making that decision at run time.

I had to go and look to find that apparently Catch doesn’t. In comparison GoogleTest supports it via GTEST_SKIP() and Boost.Test via precondition decorator. Reddit claims those are the three most popular C++ testing framowkrs which is why I’ve looked at them. Not having that feature really feels like an exception rather than a rule.

Regarding second question, the flags are there so if we can we should care about them. Users would call test being ignored even if --ignored switch is passed a bug and they would be right. Because of that, we need a solution which works with the two flags.

And this brings us to the third question, and with positive answer to the second we are now limited to an annotation. Now, whether it’s #[ignore(if = ...)] or some other syntax I don’t particularly care, I’ll just address one thing:

We already have a syntax to handle the ignored message or reason #[ignore = "not yet implemented"], and also already have a guild in rust reference.

And that syntax would stay. This RFC does not propose this syntax being removed.

clarfonthey commented 10 days ago •

This feels very much like a library solution instead of a proper language solution. If I understand correctly, the goal is to be able to distinguish between ignored and passed tests at a glance, which the current attribute does not cover.

IMHO, a more appropriate way to ignore at runtime would be to improve the existing Termination trait so that the test can return if it was ignored or not, potentially allowing for other custom statuses. Right now, you can unfortunately return ExitCode directly from tests, and this might end up being how we go it, since tests will return 0 or 1 exclusively if they pass or fail.

So, something like:

#[test] 
fn test() -> Result<(), Ignored> {
    if some_condition() {
        Err(Ignored)
    } else {
        Ok(the_test())
    }
}

mina86 commented 10 days ago

This feels very much like a library solution instead of a proper language solution. If I understand correctly, the goal is to be able to distinguish between ignored and passed tests at a glance, which the current attribute does not cover.

I’m not sure what you mean. Tests are handled by libtest so this is by definition a library change.

IMHO, a more appropriate way to ignore at runtime would be to improve the existing Termination trait so that the test can return if it was ignored or not, potentially allowing for other custom statuses. Right now, you can unfortunately return ExitCode directly from tests, and this might end up being how we go it, since tests will return 0 or 1 exclusively if they pass or fail.

That’s the ‘Returning ignored status’ from the proposal and main issue with that is that --include-ignored and --ignored libtest flags are not respected with that solution.

yanganto commented 9 days ago •

Hi @clarfonthey,

We always can do a lot of macro things in a framework to solve this issue, and if Rust can somehow change a little bit it will be better.
May I persuade you about a library solution or proper language solution with the following viewpoint?

Here is the definition of productivity of Rust on the website.

Rust has great documentation, a friendly compiler with useful error messages, and top-notch tooling — an integrated package manager and build tool, smart multi-editor support with auto-completion and type inspections, an auto-formatter, and more.

Such that we have cargo test, a wonderful testing tool with the report.

However if Rust can not provide runtime ignoring feature, people will write code like following things mention in #68007, and no mater the realthing we want to test is passed or not, the test summary show on cargo test will be pass.

#[test]
fn foo() {
    if env::var("FOO").is_none() { return; }
    let foo = env::var("FOO").unwrap();
}

This will somehow be away from the original productivity intention to provide useful error messages and top-notch tooling.
So here are my two cents, the ignoring in the test case can be provided by Rust, and the conditional things, syntax sugar, and ... can provide by the library.

clarfonthey commented 9 days ago •

I definitely didn't fully clarify what I meant, so, I'll be a bit more clear.

What I mean by #[ignore(if = function)] being a library solution is that it feels a lot more that this particular solution should be offered by crates, rather than the language itself. It definitely is a great way to support runtime ignoring for tests, but IMHO it shouldn't be the canonical way of ignoring tests, rather, just one way of doing so.

That said, the concept of marking tests as ignored at runtime isn't something that should be exclusively provided by crates, and I feel like the way we enable it should allow for a bunch of different use cases.

The reason why I mention using a return value from the function for ignoring is it fits in with an existing feature (specifically, ? in tests and returning Result in tests) and it allows more cases than the #[ignore(if = function)] syntax would. For example, if you wanted to chain multiple conditions together, there are very clear ways of doing so with a return value, but for this specific syntax, you'd have to write a new function for it.

And, as a reminder, an #[ignore_if] macro could easily be added as a proc macro that just wraps the function in a simple if.

To me, the ability to combine conditions with boolean operators is a must, and the function syntax just doesn't allow this in an easy way. A return value, although more verbose, would both support this easily in vanilla Rust, and support alternative syntaxes like the one proposed via proc macros.

Adding a few examples of some cases that would make more sense as a return value:

// "assume" macro
#[test]
fn do_something() -> Result<(), Ignored> {
    assume!(target_supports_thing()); // if (target_supports_thing()) { return Err(Ignored); }
    assume!(target_supports_other_thing());
    Ok(do_test())
}

// multiple conditions
#[test]
fn do_other_thing() -> Result<(), Ignored> {
    if target_supports_thing() && target_supports_other_thing() {
        Ok(do_test())
    } else {
        Err(Ignored)
    }
}

// try
#[test]
fn do_third_thing() -> Result<(), MyTestError> {
    do_test_for(first_value())?;
    do_test_for(second_value())
}

// (note: MyTestError implements some library trait that determines whether failure is ignore, or error)

matklad commented 9 days ago •

@clarfonthey could you spell out how return value based solutions would support --include-ignored? This I think is something which has to have support in the langue.

That is, with

#[test]
fn my_test() -> test::Result {
  if target_supports_thing() { 
    return Err(test::Ignored); 
  }
  // body of the test
  Ok(())
}

it seems that the body of the test wouldn’t get run even if user runs the test like

$ cargo test -- --ignored my_test

clarfonthey commented 9 days ago

You're right, that solution alone wouldn't work with --include-ignored; I missed the part of the discussion where that was desired, and now I agree that the existing solution is a lot more useful for that purpose.

The only way I can see a return value-based solution working for that is if the return value implemented a trait like:

trait TestCase {
    type Output: TestOutput;
    fn is_ignorable(&self) -> bool;
    fn run(self) -> Self::Output;
}

where TestOutput is the equivalent of std::process::Termination but for tests, since in looking more at it I'm not sure if this trait is used for tests.

I'm not 100% familiar with what the API for custom test harnesses would look like (if it would exist) but presumably, #[ignore]d tests would compile the run method down into a static panic when excluded to avoid compiling the tests, and non-ignorable tests would be compiled to always return false from is_ignorable.

That said, this kind of solution is definitely something that depends on the lofty goal of custom test harnesses being stable, which seems extremely far off. I don't think my arguments are worth counting against this particular implementation for that reason, unless someone comes up with a better idea or if one of my assumptions about the testing APIs is wrong.

yanganto commented 7 days ago

Hi there,

@clarfonthey you are showing that the ignore a test case will write a lot of code and this may not align with the productive goal of Rust. We do really want the runtime to ignore feature in Rust right?

By the way, there is another considering about the #[ignore_if(condition_fn)] or #[ignore(if = condition_fn)], we may take care on it.

Currently, #[ignore] or #[ignore = "reason"] is really static situation for a compiler, such that the compiler may be optimized to save resources to compile the test case. (I am not sure this is implemented or not but it is possible).

However, #[ignore_if(condition_fn)] or #[ignore(if = condition_fn)] checks the condition in runtime, so it is not static situation for a compiler, so the compiler must need to compile the test case.

In that situiation, the attributes #[ignore = "reason"], #[ignore], #[ignore_if(condition_fn)] and #[ignore(if = condition_fn)] are in different behavior, and may misleading in the future. Once people using the binary of testcase dirrectly, they will findout the issue, and confusing.

On the other hand, if #[ignore = "reason"], #[ignore] are runtime ignored to make the behavior the same, this compile can not utilize the static situation to skip building this test case.

Maybe it is good to use ignore!() or something like this inside the function to tell the difference between the runtime ignores and static compiling time ignore not only for the compiler but also clear for developers.

clarfonthey commented 7 days ago •

I should clarify, the implementation I suggested for a trait would only be an internal representation of ignorable tests; it would still require macros or attributes like the one you suggest.

An ignore! macro would be ideal, since it can act like assert and allow including ignored tests, but the main issue is for how this is communicated to the test. The macro could work like:

Original:

#[test]
fn test() {
    ignore_if!(check_condition());
    run_test();
}

Expanded code:

#[test]
fn test() {
    if !include_ignored_tests() && condition {
        send_ignore_signal();
        return;
    }
    run_test();
}

This could be via an argument to the test, maybe:

#[test]
fn test(context: &TestContext) {
    ignore_if!(context, check_condition());
    run_test();
}

Which would also work very well with RFC #2442:

#[test]
fn test(context: &TestContext) {
    context.ignore_if!(check_condition());
    run_test();
}

I guess that my main concern is that passing a function to an attribute "feels wrong" to me, but I'm not sure why. It would probably be fine to have even if another method of dynamic ignores were added in the future, since I'm not sure we want to commit to a TestContext type of some kind.

mina86 commented 7 days ago •

Currently, #[ignore] or #[ignore = "reason"] is really static situation for a compiler, such that the compiler may be optimized to save resources to compile the test case. (I am not sure this is implemented or not but it is possible).

No, this isn’t true for two reasons: First of all, --ignored and --include-ignored allow users to execute ignored tests thus the code still needs to be compiled. Second of all, the compiler needs to track all the used code in ignored test cases to properly track dead code. From compiler’s point of view, behaviour of #[ignore] and #[ignore(if = condition)] is the same: it just adds an annotation to the function. The compiler already cannot skip compilation of ignored tests.

This could be via an argument to the test, maybe:

In this case I don’t think macros are necessary. ignore_if could be a method. For now we would need ignore_if(condition) and ignore_if_with(condition, reason) methods but I think that’d be acceptable.

Of course, the main question still remains: do we care about --ignored flag working. I believe we should and if so, an annotation is the only viable solution.

I guess that my main concern is that passing a function to an attribute "feels wrong" to me, but I'm not sure why.

For what it’s worth, there is a precedent in Serde which takes functions as arguments though it takes them as strings rather than plain paths. I’m honestly not sure if there’s a reason for that.

matklad commented 4 days ago •

though it takes them as strings rather than plain paths. I’m honestly not sure if there’s a reason for that.

I think paths are just syntactically invalid in that location, only literals are allowed:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=185491f1a1d37d58cc76c2d7617b9f62

clarfonthey commented 3 days ago

though it takes them as strings rather than plain paths. I’m honestly not sure if there’s a reason for that.

I think paths are just syntactically invalid in that location, only literals are allowed:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=185491f1a1d37d58cc76c2d7617b9f62

That's not true on the latest version of Rust; it's just that serde ensures very strong compatibility with old Rust version, where it's not allowed.

kornelski commented 3 days ago •

Perhaps we're conflating two different features?

1. Current #[ignore] with ability to override it and run anyway is useful for "optional" tests. Perhaps tests that are too slow to run normally, or cause a GUI to pop-up (I have tests that require me to press a button, and they're #[ignore]d).

2. But there's a desire to skip impossible to run tests. If the architecture or program's environment doesn't support something necessary for the test (e.g. it's a test for SIMD, and it's run on a CPU without required instructions, or it's a test of filesystem, and it's running in a sandbox, etc.) then the test must be skipped. Ability to override it and run such tests anyway would be counter-productive, as it'd only cause crashes or false negatives.

Ability to override it and run such tests anyway would be counter-productive, as it'd only cause crashes or false negatives.

I'm not sure about that. I have tests that fail when my distro's package manager is running concurrently on the system. They are #[ignore] so that my package manager can cargo test && cargo install, but I use --include-ignored on the CI and in my normal development workflow.

I want my CI to keep running every tests. If somehow the CI environment changes so that the tests would get runtime-ignored, I want a CI failure. In my normal workflow I'd probably stop passing --include-ignored, but I still want some way to run the tests anyway even if I expect a failure.

there's a desire to skip impossible to run tests.

It's not just about impossible tests. My example with the package manager just make the test failure likely, and I could make the tests or code more resilient. A memory-hungry test could check the available memory before running, but be overridden by a developer who know his OS can take it.

There are lots of motivations for runtime ignore, whatever --include-ignored behavior we choose probably won't suit everybody. We have --skip and could add some version of --include-ignored-if, but that's probably overkill. I think the most versatile behavior is for --include-ignored to work the same for compiletime-ignored and runtime-ignored.

clarfonthey commented 2 days ago

Seeing more of the reasons someone might mark tests as ignored, I wonder if the best answer might simply be to make it easier to mark tests with arbitrary tags, then just allow filtering tests to allow or deny tests with certain flags.

I know that when I was developing a lot of JavaScript, Mocha had the ability to run tests by a regexp match of the test name/group, which was both helpful for filtering out weird tests or checking individual tests from the entire suite without running everything.

I'm mostly jumping to the extremely general solution since I think loads of people will have different use cases. If we had that, would "ignore" simply be another tag? Would ignore_if be best if it could add any tag, or would there be a reason to always ignore a test without being able to override it?

Aloso commented yesterday

I think that an enum return value is the simpler, more flexible approach. An attribute doesn't compose well, it introduces new syntax one has to remember, and the semantics aren't obvious; for example, a user might wonder whether a panicking predicate will cause the test to fail, or to be ignored. If we instead use established syntax (a simple if branch), then the user can understand the code easily without having to learn new syntax. Furthermore, the return value is more flexible since it allows returning from anywhere within the function. It could also support custom messages for ignored tests generated at runtime.

I agree with @kornelski that overriding #[ignore(if = not_supported_on_this_platform)] is often not desirable. As an alternative, cargo could expose the --include-ignored flag as an environment variable, so the test author can decide what to do:

#[test]
fn my_test() -> TestResult {
    if not_supported_on_this_platform() {
        // always ignores the test
        return TestResult::Ignore("not supported");
    }
    if im_feeling_lucky() {
        // only ignores the test if CARGO_TEST_INCLUDE_IGNORED is not set
        ignore_test!("I'm feeling lucky")?;
    }
    // do test...
    TestResult::Success
}

The --ignored flag is harder to support with this approach, but it's not that useful anyway and could probably be deprecated. I don't think I've ever wanted to run only ignored tests. Something that would be much more useful is if you could organize your tests into groups and sub-groups, and choose which groups you want to include or ignore when running tests. But that's just a random idea.

About the test crate being unstable: This can be changed. It's possible to stabilize test without stabilizing any of the items in the crate. Or am I missing something?

Passing the "should ignored tests be included ?" information to the runtime seems like a good way to handle different reasons for ignored, I like it. However, it's not tied to the "attribute vs termination" debate, as Aloso's example could be written as:

fn not_supported_on_this_platform(_include_ignored: bool) -> Option<String> {
    /// answer regardless of include-ignored flag
   (!is_linux()).then(|| "Only works on Linux")
}

fn low_ram(include_ignored: bool) -> Option<String> {
    /// respect include-ignored flag
   (ram() < 64 && !include_ignored).then(|| "You need 64G of RAM")
}

#[test]
#[ignore(if = not_supported_on_this_platform)]
fn my_linux_test() {
    assert!(...);
}

#[ignore(if = low_ram)]
fn my_heavy_test() {
    assert!(...);
}

It seems to me that both are as versatile ? I prefer the attribute version as it seems less verbose/repetitive and closer to the existing functionality, but either would work.