-
Notifications
You must be signed in to change notification settings - Fork 29
feat(minor): Support range and relative in static threshold files [3/4]
#330
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
base: main
Are you sure you want to change the base?
Conversation
range and relative in static threshold files [3/4]
| precondition( | ||
| self.containsAnyValue, | ||
| "RelativeOrRange must contain either a relative or range, but contains neither" | ||
| ) |
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.
In the next PR I've relaxed all these preconditions and made them just print warnings.
I've noticed the benchmark-tool running doesn't work too well with preconditions. It'll be hard to debug them.
| if let threshold = thresholdsFromCode.absolute[percentile], | ||
| !(-threshold...threshold).contains(absoluteDifference) | ||
| { | ||
| let deviation = ThresholdDeviation( |
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.
Not quite sure about these SwiftLint errors/warnings. Some of them doesn't play well with swift-format, like this one.
Let me know what I should do.
| debugDescription: """ | ||
| RelativeOrRange thresholds object does not contain either a valid relative or range. | ||
| For relative thresholds, both 'base' (Int) and 'tolerancePercentage' (Double) must be present and valid. | ||
| For range thresholds, both 'min' (Int) and 'max' (Int) must be present and valid. | ||
| You can declare both relative and range in the same object together, or just one of them. | ||
| Example: { "min": 90, "max": 110 } | ||
| Example: { "base": 115, "tolerancePercentage": 5.5 } | ||
| Example: { "base": 115, "tolerancePercentage": 4.5, "min": 90, "max": 110 } | ||
| """ |
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.
Some info.
This is also backward-compatible. It'll try to read an Int first and if it fails then it tries to read the new RelativeOrRange.
04291aa to
f441e9e
Compare
998e8b7 to
3137c2c
Compare
|
@hassila tried to properly rebase these 2 remaining PRs. |
Description
Only the last commit is related to this PR. The other commits are PR [1/4] and [2/4]. We'll have to merge these in order.
Partially resolves #327.
Adds support for
min/max"range"s andbase/tolerancePercentage"relative"s in static threshold files.Waiting on your feedback first. When changes are settled I can add the docs as well.
Feel free to review on your own schedule.
EDIT: now that things are somewhat stable, i notice the next PR ([4/4]) has made a decent amount of changes to this PR's changes.
I could try to rebase things and all but that'll be a bit of trouble. Or we could skip this PR and review the next one which also includes this PR but with more changes. Not sure.
How Has This Been Tested?
Manually in my PRs. I'll be happy to add unit tests where you see appropriate as well.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package