Bug 226894 – [resultsdbpy] Add results-summary API

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Gambrell]

Adding functionality to predict the pass, fail, crash, etc percentages of a given test to the endpoint /api/confidence

[Radar WebKit Bug Importer]

<rdar://problem/79155181>

[Chris Gambrell]

Created attachment 431113 [details]
Patch

[Chris Gambrell]

Created attachment 431125 [details]
Patch

[Jonathan Bedard]

Comment on attachment 431125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431125&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:182
> +            newer_data = []

Want to call out a problem here Chris and I discovered. Under the current code, this is always going to return the most recent `limit` of test results which occurred after the specified commit. The problem with this, is that it will still return the most recent test results even if the specified commit is ancient. I need to double check if there is a way for us to resolve in our Cassandra query, because I'm not sure there is.

[Chris Gambrell]

Created attachment 431129 [details]
Patch

[Chris Gambrell]

Created attachment 431130 [details]
Patch

[Chris Gambrell]

Created attachment 431221 [details]
Patch

[Jonathan Bedard]

Created attachment 435221 [details]
Patch

[Aakash Jain]

Comment on attachment 435221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435221&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:176
> +        uds = []

better to name it uuids

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:199
> +            return abort(400, description='No results for configuration in range')

This error is non human-friendly. Should make it something which is easy to understand. 
If the test is not found, the error message should specify that. If the commit is not found, error message should indicate that.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:363
> +                '/api/aggregate-results/&ltsuite&gt/&lttest&gt',

aggregate-results doesn't sounds like a right name for this api from user's perspective

[Jonathan Bedard]

Created attachment 435269 [details]
Patch

[Jonathan Bedard]

Comment on attachment 435221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435221&action=review

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:176
>> +        uds = []
> 
> better to name it uuids

Need to delete this, was just a variable for testing.

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:363
>> +                '/api/aggregate-results/&ltsuite&gt/&lttest&gt',
> 
> aggregate-results doesn't sounds like a right name for this api from user's perspective

Discussing a better API name with Aakash, results-status, results-confidence, results-prediction are some candidates

[Jonathan Bedard]

Created attachment 435272 [details]
Patch

[Aakash Jain]

Comment on attachment 435272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435272&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:363
> +                '/api/aggregate-results/&ltsuite&gt/&lttest&gt',

let's call this results-summary endpoint

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:367
> +                    `Compute the combined results of a give test by aggregating results from a set of runs surrounding a specific commit:`,

Nit: give => given

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:527
> +            codeBlock('include_expectations=Flase'),

Nit: Flase

[Jonathan Bedard]

Created attachment 435288 [details]
Patch

[Aakash Jain]

rs=me with the above comments incorporated

[Aakash Jain]

Comment on attachment 435288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435288&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:203
> +        for key_i in response.keys():

Nit: can rename key_i to key, and key_ii to key_i or something meaningful

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:206
> +                    response[key_i][key_ii] = 100 * response[key_i][key_ii] // (aggregate or 1)

Nit: // => #

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:364
> +                ['Branch', 'Configuration', 'Include Expectations', 'Limit', 'Repository', 'Ref', 'UUID'],

Rename: 'Include Expectations' => include_expectations. Better not to change the keyword.

[Jonathan Bedard]

Created attachment 435291 [details]
Patch

[Jonathan Bedard]

Comment on attachment 435288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435288&action=review

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/test_controller.py:206
>> +                    response[key_i][key_ii] = 100 * response[key_i][key_ii] // (aggregate or 1)
> 
> Nit: // => #

`//` is integer divide

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/documentation.html:364
>> +                ['Branch', 'Configuration', 'Include Expectations', 'Limit', 'Repository', 'Ref', 'UUID'],
> 
> Rename: 'Include Expectations' => include_expectations. Better not to change the keyword.

While we could do this, this ends up really breaking the way we use these parameters.

[Jonathan Bedard]

Created attachment 435299 [details]
Patch

[Jonathan Bedard]

Committed r280869 (240408@main): <https://commits.webkit.org/240408@main>