Allow '/' in test name (rust) #26
Conversation
* npm run test wouldn't work until i updated `write.ts` * Tests added * add cargo and target to ignore * add workflow * Make working benchmark
|
Note that I took the example from https://bheisler.github.io/criterion.rs/book/user_guide/comparing_functions.html to create the benchmark_group. |
| } | ||
|
|
||
| fn bench_fib_20(c: &mut Criterion) { | ||
| c.bench_function("BenchFib20", move |b| { |
There was a problem hiding this comment.
Note that we can add whitespaces and other characters to the bench title that would make the title doesn't match with the extract regex.
I didn't add it because I am not sure if you want to add whitespaces and possibly other characters to the regex and because this can be avoided by the owner of the repo that is running this action by just renaming the bench title.
There was a problem hiding this comment.
ok. When someone complains about that, let's consider to support spaces by making output parser smarter.
Use criterion rs
|
Is this PR intended to be merged after #10 is merged? |
|
No, I think that #10 is not needed anymore. |
|
Yup, #10 can be closed |
|
Since I'm currently not a user of Criterion.rs, I'm not understanding the situation. Why is #10 not necessary? Did it change the output format? |
|
Yes, we got criterion to add support for output format that is compatible with bencher which supports this action. |
|
You can see in the changelog of criterion.rs for the version 0.3.2 that it was added support to get a bencher-like output |
|
Oh, that's good news. I did not know that. Then what we should add to our implementation is only this line and it looks good. I'll merge this. |
The latest version of criterion.rs allow us to generate a
bencherlike output withcargo bench -- --output-format bencherso we can use the current extractCargoResult to parse them.If you see the way that the report is done there, the title of the test can have '/', therefore I changed a the regex in order to allow names with that character. You can see that working in https://github.com/morenol/kvs/pull/2/checks?check_run_id=620736396
I am adding here the key parts, just to save you time:
https://github.com/bheisler/criterion.rs/blob/7e2d02b5d89ca87a1d2e5c8ddbce33c2077a51f6/src/report.rs#L752
https://github.com/bheisler/criterion.rs/blob/7e2d02b5d89ca87a1d2e5c8ddbce33c2077a51f6/src/report.rs#L228
https://github.com/bheisler/criterion.rs/blob/7e2d02b5d89ca87a1d2e5c8ddbce33c2077a51f6/src/report.rs#L123
This is related to: #8
Note that I took and squashed the changes from #10 (except by the readme change)