Fix rust benchmark #923
Fix rust benchmark #923
Conversation
UebelAndre
force-pushed
the
fix-rust-benchmark
branch
9 times, most recently
from
12fa172
to
034d550
Compare
UebelAndre
force-pushed
the
fix-rust-benchmark
branch
from
d38f53f
to
892d15f
Compare
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
| @@ -13,7 +13,7 @@ | |||
| # limitations under the License. | |||
|
|
|||
| load("@rules_rust//rust:rust.bzl", "rust_benchmark") | |||
| load("//benchmarking/private:bench_runner.bzl", example_bench_runner_test = "bench_runner_test") | |||
| load("//benchmarking/private:example_bench_runner.bzl", "example_bench_runner_test") | |||
There was a problem hiding this comment.
Sorry, what I meant was to literally remove the example_bench_runner_test macro, and have a rust_test each place it's used...
There was a problem hiding this comment.
Ah, I feel like the existence of it kinda crosses the line between example and integration test. I'd like it to be clear that the target there has nothing to do with the use of rust_benchmark but since I suspect the common use case is going to be criterion, which I kinda don't want to put in the test package, I opted to make this target. With that being said, I don't mind deleting the macro, I just want it to be clear that it's not something users should need to care about
There was a problem hiding this comment.
Though, I can probably move the tests outside each package...
There was a problem hiding this comment.
In that case I'd be pretty tempted to move this to the test package - I don't really mind if we end up duplicating the actually rust_benchmark between examples and test...
There was a problem hiding this comment.
I ended up moving it. Should there still be something in examples? Also, I'm kinda unhappy about having a test with a bunch of external dependencies. I'd really hate for a test to fail because something happened to some network resource and don't want to have iteration loops where your test failed but to thoroughly test your changes you need to build a massive application. This particular case wouldn't qualify as "massive" to me but it is in comparison to the existing tests and introducing dependencies like this can be a slippery slope.
This is a revival of #532 (Thanks @djmarcin!)
Changes
use_libtest_harnessattribute torust_benchmarkallowing for the use of criterionrust_binaryfor invoking benchmarks with a controlled set of arguments