Bug 233800 – [Tools] Add script to query results database

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Lauro Moura]

[Tools] Add script to query results database

[Lauro Moura]

Created attachment 445805 [details]
Patch

[Lauro Moura]

Created attachment 445847 [details]
Updated patch

It was missing the IMAGE expectation from the list of possible expected values (it was based on a pytest script for webdriver)

[Jonathan Bedard]

Comment on attachment 445847 [details]
Updated patch

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

> Tools/Scripts/webkit-test-results:54
> +    "TIMEOUT",  # Put timeout as more severe than ERROR as they often are harder to debug

Shouldn't crash be in here too?

Totally fine having this script own this, but results database's list is here:
Tools/Scripts/libraries/resultsdbpy/resultsdbpy/model/test_context.py

We probably should do some refactoring on the above since if you imported that, you'd have a dependency on cassandra which is silly

> Tools/Scripts/webkit-test-results:107
> +}

I wonder if the better implementation is to have this set of query aliases, but to treat anything that doesn't match a query alias as the query. So -b=gtk-release-x11 and -b='platform=GTK&style=release&version_name=Xvfb&version=5.5.0' would give you the same thing. It would be pretty easy to tell if the user meant a -b argument to be a query because it would have to have a '=' in it.

> Tools/Scripts/webkit-test-results:154
> +        # Usually order is zero, but let's add it anyway as it's the actual

Minor note on what's going on under the hood: "order" lets us support ordering in commits that are landed at exactly the same time. In subversion, this is impossible, but it is possible in git, so it's likely order won't always be zero when we ditch SVN.

If you wanted to, the Commit object from webkitscmpy can do this, but you've managed to avoid anything other than built-in dependencies so far, so I won't ask that you add any.

> Tools/Scripts/webkit-test-results:190
> +    endpoint = "api/results/layout-tests"

Seems like supporting other test suites would just be a matter of making an argparser for it, right?

[Radar WebKit Bug Importer]

<rdar://problem/86305620>

[Lauro Moura]

Created attachment 452707 [details]
Updated patch

[Lauro Moura]

(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 445847 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445847&action=review
> 
> > Tools/Scripts/webkit-test-results:54
> > +    "TIMEOUT",  # Put timeout as more severe than ERROR as they often are harder to debug
> 
> Shouldn't crash be in here too?

Nice catch.

> 
> Totally fine having this script own this, but results database's list is
> here:
> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/model/test_context.py
> 
> We probably should do some refactoring on the above since if you imported
> that, you'd have a dependency on cassandra which is silly

One point that I had in mind to reorder the expectations for this script was that ERROR would be "worse" than the regular failures (TEXT, IMAGE). Meanwhile, both webkitpy.results.upload.Expectations and the resultsdb.model.test_context.Expectations place ERROR as more severe than only PASS and WARNING.

For example, this scripts sees a test moving from FAIL to ERROR as a regression, while in the existing classes it kinda "improved". Does this make sense?

In any case, if we keep the same scale of severity, I could use webkitpy.results.upload.Exception instead of importing the resultsdbpy.

> 
> > Tools/Scripts/webkit-test-results:107
> > +}
> 
> I wonder if the better implementation is to have this set of query aliases,
> but to treat anything that doesn't match a query alias as the query. So
> -b=gtk-release-x11 and
> -b='platform=GTK&style=release&version_name=Xvfb&version=5.5.0' would give
> you the same thing. It would be pretty easy to tell if the user meant a -b
> argument to be a query because it would have to have a '=' in it.

Great suggestion. Updated.

> 
> > Tools/Scripts/webkit-test-results:154
> > +        # Usually order is zero, but let's add it anyway as it's the actual
> 
> Minor note on what's going on under the hood: "order" lets us support
> ordering in commits that are landed at exactly the same time. In subversion,
> this is impossible, but it is possible in git, so it's likely order won't
> always be zero when we ditch SVN.

I've updated the comment to mention this.

> 
> If you wanted to, the Commit object from webkitscmpy can do this, but you've
> managed to avoid anything other than built-in dependencies so far, so I
> won't ask that you add any.

Yep. Like the resultsdbpy above, I tried to avoid pulling bit dependencies on this script.

> 
> > Tools/Scripts/webkit-test-results:190
> > +    endpoint = "api/results/layout-tests"
> 
> Seems like supporting other test suites would just be a matter of making an
> argparser for it, right?

Likely. I didn't add support for it yet as, IIRC, only layout-tests results are properly processed into the result database by GTK/WPE.

[Jonathan Bedard]

Comment on attachment 452707 [details]
Updated patch

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

> Tools/Scripts/webkit-test-results:342
> +    parser.add_argument("--list-bots", action="store_true", help="List predefined bots")

This option still requires "-b" to run, but that doesn't seem deliberate

[Lauro Moura]

Created attachment 453938 [details]
Updated patch

Nice catch. I've grouped both -b and --list-bots into a mutually exclusive group. Other minor edits include aborting if no bot is passed when trying to get the history of a test and avoiding an exception when there are multiple expectations (e.g. TIMEOUT FAIL)

[EWS]

Committed r291067 (248239@main): <https://commits.webkit.org/248239@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453938 [details].