Better error message needed when user creates a bench rather than a benches dir

[gilescope]

A common error with benchmarking is to call the dir bench rather than benches.

This is the error message a user currently gets:

error: failed to parse manifest at `<snip>/Cargo.toml`

Caused by:
  can't find `my_benchmark` bench, specify bench.path

It fails the esteban test as it doesn't hint as to what we've done wrong.

At a minimum can we change it to this please?

Caused by:
  can't find `benches/my_benchmark` bench, specify bench.path

That way they stand a chance of spotting there's an issue. Personally I think it's worth testing for a bench dir and having an explicit message because so many people must do this and then kick themselves when the penny drops.

[CPerezz]

That's weird. I just implemented this test and it passes:

#[cargo_test]
fn external_bench_warns_wrong_path() {
    if !is_nightly() {
        return;
    }

    let p = project()
        .file(
            "src/lib.rs",
            r#"
                #![feature(test)]
                #[cfg(test)]
                extern crate test;

                pub fn get_hello() -> &'static str { "Hello" }
            "#,
        )
        .file(
            "./bench/external.rs",
            r#"
                #![feature(test)]
                #[allow(unused_extern_crates)]
                extern crate foo;
                extern crate test;

                #[bench]
                fn external_bench(_b: &mut test::Bencher) {()}
            "#,
        )
        .build();

    p.cargo("bench")
        .with_stderr_contains("can't find `[..]` bench, specify `[..]`")
        .run();
}

@gilescope was the [[bench]] section configured on any way in Cargo.toml? ie. harness=false or anything similar?
Maybe I implemented the test wrong. But seems that bench gets digested correctly by cargo if you use the default bencher.

[gilescope]

In the above case it's not erroring but not running any benchmarks either I think?

Looking back I was using criterion (sorry) and so had this in the Cargo.toml:

[[bench]]
name = "my_benchmark"
harness = false

That at least errored (but didn't point out what the error was). I think Criterion could do with an upgradeed error message (assuming the error message above is ultimately created by criterion) and rust's bench could do with emitting a warning that while no benchmarks were found there's suspiciously a bench dir in existence and did you really mean benches?

[CPerezz]

In the above case it's not erroring but not running any benchmarks either I think?

It is, the bench runs and works. that's why the test fails if I include .with_stderr_contains("can't find [..]bench, specify[..]")

So basically I asume (maybe @ehuss or @Eh2406 can bring some light here) that adding this [[bench]] section setting harness = false as said in the docs this makes cargo pass the --test flag to rustc which causes the import of the libtest crate. Later it says about the harness:

If set to false, then you are responsible for defining a main() function to run tests and benchmarks.

And I guess that this is the problem. When the Criterion main.rs is executed here the error that they return is not explicit as it should I agree.

Again I might be wrong, but looks to me that this can be closed and the issue should be opened in Criterion instead.

[ehuss]

@CPerezz I think you may be misunderstanding the issue. The issue is that specifying a bench target like this in Cargo.toml:

[[bench]]
name = "mybench"

And you place the file in bench/mybench.rs, that it returns a confusing error message. The expected location is benches/mybench.rs.

I think there are two parts to improve the message here:

1. If path is not specified, the error message could include the implicit path in the message, maybe something like this:

Caused by:
  can't find `mybench` bench at `benches/mybench.rs`, specify bench.path if you want to use a non-default path

2. If path is not specified, but there is a file found at a "commonly wrong location" like bench/<name>.rs, there could be a note attached suggesting renaming the directory as another option to fix it.

It would be nice if these better error messages worked for all of the target types (test → tests and example → examples and maybe src/bins → src/bin).

[CPerezz]

@ehuss I understood the problem. But I wrongly thought that was harness the problematic option, not the name itself. When (as it makes sense now) it was.

Will start working on it if it's ok with you! I'll probably push a PR for 1. that addresses your final suggestion also (not bin al least for now) and will open another issue/PR for 2.

[djmitche]

@CPerezz are you still planning to work on this? If not, I could..

[tverghis]

@djmitche @CPerezz I'd like to claim this if neither of you are actively working on this issue

[djmitche]

I completely forgot about this after polling @CPerezz. I think you're clear to continue!

[CPerezz]

Hey. Feel free to work on it! I´ve been extremely bussy.. Apologies

…

On Fri, 10 Sept 2021 at 19:57, Dustin J. Mitchell ***@***.***> wrote: I completely forgot about this after polling @CPerezz <https://github.com/CPerezz>. I think you're clear to continue! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI4J4HRWANCWCHAXL7A7ZEDUBJBHDANCNFSM4VH7JGXQ> .