-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Add validator for benchmarks that contains null runtime #2771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
41b066f
to
4b61ffb
Compare
I think the UX needs to be improved around this, but this is ok as a stop-gap for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
I've updated runtime validation logics (457330d) Example validation error messages
|
Co-authored-by: Tim Cassell <cassell.timothy@gmail.com>
ef841f8
to
ea68e83
Compare
It's not related to this PR thought.
|
Just thought about in-process toolchains after I merged. The current runtime behavior is correct on those toolchains, so they should not be included as part of the validation. @filzrev |
This PR add validator to handle issue that is reported #2609.
What's changed in this PR
RuntimeValidator.cs
for benchmarks that have multiple runtimes and contains null.Background
When benchmark don't contains
Runtime
characteristic. (e.g. When specifyWithToolChain() only
)BenchmarkCase::GetRuntime returns current runtime instead of actual runtime that used by benchmark.
Normally,
Runtime
column is hidden on table because all benchmarks have same runtime value.So it's not a problem. But when mixed
WithRuntime
settings. it display wrong runtime.So it need to add validator to check benchmarks that contains multiple runtimes and contains null.
RuntimeValidator's error message