Bug 203905 – results.webkit.org: List failing tests for criteria

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We should be able to list tests which are failing given a revision, or revision range.

[Jonathan Bedard]

Created attachment 382945 [details]
Patch

[Jonathan Bedard]

Before I actually deploy this to a staging instance, is this the sort of thing we're looking for?

[Zhifei Fang]

Comment on attachment 382945 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/templates/documentation.html:368
> +                    '            "suite.sub-2.test-1": "PASS",\n' +

Discussed with Jonathan personally, we should not provide unexpected pass results here, this make this API confused. And in the collapsed list, user cannot tell which one is a new pass and which one is the real failure, therefore, we should get rid of new pass results.

[Jonathan Bedard]

Created attachment 382981 [details]
Patch

[Jonathan Bedard]

Created attachment 383020 [details]
Patch

[Aakash Jain]

Comment on attachment 383020 [details]
Patch

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

> Tools/ChangeLog:49
> +        For the /failure endpoint.

Nit 'For' => 'for'

> Tools/resultsdbpy/resultsdbpy/view/templates/documentation.html:351
> +                ['Aggregation', 'Branch', 'Branch', 'Configuration', 'Limit', 'Repository', 'Time', 'UUID'],

Is the 'Branch' duplication intentional?

> Tools/resultsdbpy/resultsdbpy/view/templates/documentation.html:353
> +                    `Returns a list of tests which failed during test runs matching the specified criteria. When collapsed, these listed will be a sorted list looking like this:`,

Nit: 'these listed' => 'these results'

> Tools/resultsdbpy/resultsdbpy/view/templates/documentation.html:379
> +                    `where &ltconfiguration-object-a&gt and &ltconfiguration-object-b&gt are both ${localLink(['Query Parameters', 'Configuration'], 'configuration objects')}'.`,

extra ' here after configuration objects.

> Tools/resultsdbpy/resultsdbpy/view/templates/documentation.html:461
> +            `Because the results database distinguishes between expected and unexpected failures, endpoints preforming aggregation will often filter out expected failures, and flag unexpected passes. To modify the behavior of these algorithms, these endpoints will support the unexpected flag:`,

typo: preforming

[Aakash Jain]

Comment on attachment 383020 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/controller/failure_controller.py:69
> +            specified_commits = sum([1 if element else 0 for element in [begin, end]])

num_specified_commits or specified_commits_counts might be more descriptive name.

> Tools/resultsdbpy/resultsdbpy/controller/failure_controller.py:70
> +            specified_timestamps = sum([1 if element else 0 for element in [begin_query_time, end_query_time]])

Ditto. num_specified_timestamps or specified_timestamps_counts might be more descriptive name.

> Tools/resultsdbpy/resultsdbpy/controller/failure_controller.py:77
> +

Nit: is this newline required?

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:54
> +    class TestFailuresByCommit(TestFailuresBase):

aren't we storing the revision id in these tables?

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:70
> +        start_time = columns.DateTime(primary_key=True, required=True)

Nit: please maintain same order of variable declaration in all these 4 classes.

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:84
> +            self.cassandra.create_table(self.UnexpectedTestFailuresByCommit)

What's the benefit of adding two additional tables for unexpectedTestFailures? Can't we store this information in TestFailuresByCommit, TestFailuresByStartTime.

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:88
> +        timestamp = timestamp or time.time()

time.time() is not an instance of datetime (which is being verified in next line), why not just initialize with correct value.

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:97
> +                timestamp = calendar.timegm(timestamp.timetuple())

why converting twice? why not just convert it above (in line 90) to required type?

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:160
> +            raise TypeError(f'Expected type {str}, got {type(suite)}')

This error message should specify which variable has incorrect type.

> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:162
> +        def get_time(time):

This method should be in some common class (like Util), as it's used in multiple classes. But that can be done in a separate patch.

[Jonathan Bedard]

Comment on attachment 383020 [details]
Patch

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

>> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:54
>> +    class TestFailuresByCommit(TestFailuresBase):
> 
> aren't we storing the revision id in these tables?

We are, but order maters here. It changes the way we search. See bellow comment about why we don't maintain the same order.

>> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:70
>> +        start_time = columns.DateTime(primary_key=True, required=True)
> 
> Nit: please maintain same order of variable declaration in all these 4 classes.

The fact that the order is different is exactly why we have two tables.

The first table is defined UUID then time, the second by time then UUID. This allows us to support both queries like 'find me all tests which ran on commits from yesterday' and 'find me all tests which ran yesterday'

>> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:84
>> +            self.cassandra.create_table(self.UnexpectedTestFailuresByCommit)
> 
> What's the benefit of adding two additional tables for unexpectedTestFailures? Can't we store this information in TestFailuresByCommit, TestFailuresByStartTime.

See above comment about why order is different.

>> Tools/resultsdbpy/resultsdbpy/model/failure_context.py:97
>> +                timestamp = calendar.timegm(timestamp.timetuple())
> 
> why converting twice? why not just convert it above (in line 90) to required type?

Because I wasn't paying attention. It's this line that's correct, the previous one is wrong.

[Aakash Jain]

Comment on attachment 383020 [details]
Patch

rs=me, with the fixes for the minor issues mentioned above.

[Jonathan Bedard]

Created attachment 383168 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383168 [details]
Patch

Clearing flags on attachment: 383168

Committed r252272: <https://trac.webkit.org/changeset/252272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57040603>