Update docs after repos merge#1132
Conversation
…e files used by networking benchmarks)
src/benchmarks/micro/Categories.cs
Outdated
| /// benchmarks belonging to this category are executed for CoreFX CI jobs | ||
| /// </summary> | ||
| public const string CoreFX = "CoreFX"; | ||
| public const string Libraries = "CoreFX"; |
There was a problem hiding this comment.
@billwert @DrewScoggins I have changed the folder structure and renamed the categories (CoreFX -> Libraries, CoreCLR -> Runtime) but did not change the... values of the constants.
So in the code we now have:
[BenchmarkCategory(Categories.Runtime, Categories.Libraries)]But the values remained CoreFX and CoreCLR. I did that to make sure that we don't change the real categories names which I assume would affect the naming in the Reporting Service. Please let me know if this is good or bad idea.
There was a problem hiding this comment.
@DrewScoggins Can you confirm whether or not category is part of test identity? I think it is, but this leads to interesting questions (such as if someone adds a category will they break us)?
I think this is a fine change for now, leaving the values the same. I'm a little worried that we're going to have the impetus to change these again the next time we rename teams, and would like to avoid that.
There was a problem hiding this comment.
We store the test category in a side table, so it is not part of the main identity. Updating the category for the test will just add this category as one of many that are applied to the test. So this would not break anything in the reporting side.
There was a problem hiding this comment.
Thanks for the explanation. I've changed the values as well. So now CoreCLR and CoreFX names are not really used anymore
| *.dbmdl | ||
| *.dbproj.schemaview | ||
| *.jfm | ||
| *.pfx |
There was a problem hiding this comment.
we actually use pfx files in this repo (certificates used by networking benchmarks)
billwert
left a comment
billwert
left a comment
There was a problem hiding this comment.
Overall looks pretty good. Left a handful of comments/questions.
src/benchmarks/micro/Categories.cs
Outdated
| /// benchmarks belonging to this category are executed for CoreFX CI jobs | ||
| /// </summary> | ||
| public const string CoreFX = "CoreFX"; | ||
| public const string Libraries = "CoreFX"; |
There was a problem hiding this comment.
@DrewScoggins Can you confirm whether or not category is part of test identity? I think it is, but this leads to interesting questions (such as if someone adds a category will they break us)?
I think this is a fine change for now, leaving the values the same. I'm a little worried that we're going to have the impetus to change these again the next time we rename teams, and would like to avoid that.
Co-Authored-By: Bill Wert <billwert@microsoft.com>
|
@billwert Thank you for the review! I've applied all the suggestions. PTAL |
|
LGTM! (Sorry, was OOF sick yesterday.) |
|
@filipnavara as far as I remember you are using categories names to filter the benchmarks when running them for Mono. After this PR got merged you should change your filters:
|
|
@adamsitnik Thanks for heads up. I already noticed my builds were failing and fixed it up yesterday. :) |
5 participants
Fixes #990