Bug 201321 – run-webkit-tests: Report results archive to results.webkit.org

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

After https://bugs.webkit.org/show_bug.cgi?id=201100 lands, we should upload test result archives to the results database for long-term storage.

[Jonathan Bedard]

Created attachment 377658 [details]
Patch

[Jonathan Bedard]

Created attachment 377706 [details]
Patch

[Aakash Jain]

Comment on attachment 377706 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:249
> +        start_time = int(time.time())

start_time is already defined on line 217.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:287
> +                    timestamp=start_time,

what does timestamp indicate here? Should we use time.time() directly here, so that for each upload we have distinct timestamp?

Also, is this a drive-by fix or actually needed for this patch?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:321
> +            self._printer.write_update('Preparing upload test archive ...')

'Preparing upload test archive' reads weird. I know you copied it from above 'Preparing upload data' message. Maybe you can re-phrase this message to something like: 'Preparing to upload test archive ...')

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:325
> +                shutil.make_archive(archive, 'zip', self._results_directory)

should we clean up this archive after successful upload?  or is it cleaned-up somewhere else?
If not, what if next run silently fails to overwrite this file and we upload archive from previous run? that might be hard to find out.

> Tools/Scripts/webkitpy/results/upload.py:35
> +    ARCHIVE_UPLOAD_ENDPOINT = '{}/archive'.format(UPLOAD_ENDPOINT)

This is ok, but might be more clear to simply use '/api/upload/archive'.

> Tools/Scripts/webkitpy/results/upload.py:104
> +        self.timestamp = int(timestamp or time.time())

why int and not float (default)?
and is the precision of 'seconds' enough, or should be include milliseconds?

> Tools/Scripts/webkitpy/results/upload.py:199
> +                url + self.ARCHIVE_UPLOAD_ENDPOINT,

so we are constructing the url here. And the url passed in options.report_urls wasn't the url, it was only the hostname. That seems misleading. 

options.report_urls should be renamed to something like: options.report_hostnames

Similarly parameter to this method shouldn't be called 'url', it should be 'hostname'. If it's called 'url', i would except it to take the actual url.
Something like: https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/net/statusserver.py#L47

> Tools/Scripts/webkitpy/results/upload.py:212
> +            log_line_func(' ' * 4 + 'Error uploading archive to {}:'.format(url))

Nit: misplaced :

> Tools/Scripts/webkitpy/results/upload.py:213
> +            log_line_func(' ' * 8 + response.json()['description'])

is response.json() guaranteed to have 'description'? If not, it might be better to use .get(), so that we don't get an exception while printing an error message.

> Tools/Scripts/webkitpy/results/upload_unittest.py:229
> +            self.assertTrue(upload.upload_archive('https://webkit.org/results', archive='content', log_line_func=lambda _: None))

it seems upload_archive() expects a hostname, so this url should be modified accordingly as well (although it wouldn't really matter for unit-test.

> Tools/Scripts/webkitpy/results/upload_unittest.py:239
> +            self.assertFalse(upload.upload_archive('https://webkit.org/results', archive='content', log_line_func=lambda _: None))

Can we assert the error message ('Error uploading archive to {}') as well?

[Jonathan Bedard]

Comment on attachment 377706 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:287
>> +                    timestamp=start_time,
> 
> what does timestamp indicate here? Should we use time.time() directly here, so that for each upload we have distinct timestamp?
> 
> Also, is this a drive-by fix or actually needed for this patch?

It's the key we use in the results database to differentiate test runs.

We don't want each upload from a single test run to have a distinct timestamp, actually. The point of this change is so that the json results can be readily paired with their associated archive because they will have the same timestamp. Ie, if I see a failure which occurred with configuration X at timestamp Y, I can directly use configuration X and timestamp Y to look at the archived results from that same test run.

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:325
>> +                shutil.make_archive(archive, 'zip', self._results_directory)
> 
> should we clean up this archive after successful upload?  or is it cleaned-up somewhere else?
> If not, what if next run silently fails to overwrite this file and we upload archive from previous run? that might be hard to find out.

'with self._filesystem.mkdtemp() as temp:' means that we're making the archive in a temporary folder, that decorator removes the folder upon exiting the context.

>> Tools/Scripts/webkitpy/results/upload.py:104
>> +        self.timestamp = int(timestamp or time.time())
> 
> why int and not float (default)?
> and is the precision of 'seconds' enough, or should be include milliseconds?

Because we're passing this value to the results database which stores this value as an integer.

We can't really change the precision without a database migration (which is prohibitively expensive, to the point of being essentially impossible...one of the downsides of Cassandra). More to your point, though, we're using the timestamp to distinguish test runs with identical configurations and commits, so seconds is plenty of precision.

[Jonathan Bedard]

Created attachment 378246 [details]
Patch

[Aakash Jain]

Comment on attachment 378246 [details]
Patch

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

r+ after the comments are addressed.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:294
> +                for host_name in self._options.report_urls:

Should report_url also renamed to report_hostnames? Maybe in a separate patch.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:295
> +                    self._printer.write_update('Uploading to {} ...'.format(host_name))

should it be host_name or hostname?

> Tools/Scripts/webkitpy/results/upload.py:35
> +    ARCHIVE_UPLOAD_ENDPOINT = '/api/upload/archive'.format(UPLOAD_ENDPOINT)

seems like you missed removing .format

> Tools/Scripts/webkitpy/results/upload.py:170
> +    def upload(self, host, log_line_func=lambda val: sys.stdout.write(val + '\n')):

what is better, host or hostname, maybe hostname to maintain consistency?

> Tools/Scripts/webkitpy/results/upload.py:171
>          try:

we should probably check here that host is not None/empty, and print proper error message, rather than not-so-user-friendly error about failed upload.

> Tools/Scripts/webkitpy/results/upload.py:188
> +    def upload_archive(self, host, archive, log_line_func=lambda val: sys.stdout.write(val + '\n')):

Ditto for host vs hostname and validation.

> Tools/Scripts/webkitpy/results/upload_unittest.py:141
>  

We should probably add a unit-test with empty URL, after adding the error-checking in the upload() method.

[Jonathan Bedard]

Comment on attachment 378246 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:294
>> +                for host_name in self._options.report_urls:
> 
> Should report_url also renamed to report_hostnames? Maybe in a separate patch.

That definitely needs to be a separate patch because layout tests aren't the only test suite that has that option.

>> Tools/Scripts/webkitpy/results/upload.py:171
>>          try:
> 
> we should probably check here that host is not None/empty, and print proper error message, rather than not-so-user-friendly error about failed upload.

We don't need an explicit check, an empty hostname would trigger the error message on line 174

[Jonathan Bedard]

Created attachment 378381 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378381 [details]
Patch for landing

Clearing flags on attachment: 378381

Committed r249652: <https://trac.webkit.org/changeset/249652>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55190632>