Bug 195755 - webkitpy: Upload test results
Summary: webkitpy: Upload test results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 12:09 PDT by Jonathan Bedard
Modified: 2019-03-15 19:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (36.80 KB, patch)
2019-03-14 12:14 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (57.25 KB, patch)
2019-03-14 17:12 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (58.84 KB, patch)
2019-03-14 17:27 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (58.95 KB, patch)
2019-03-15 08:13 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (59.75 KB, patch)
2019-03-15 08:31 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (60.09 KB, patch)
2019-03-15 08:53 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (60.12 KB, patch)
2019-03-15 08:58 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (60.35 KB, patch)
2019-03-15 09:16 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (40.51 KB, patch)
2019-03-15 10:46 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (40.51 KB, patch)
2019-03-15 11:10 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (40.54 KB, patch)
2019-03-15 11:29 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (39.49 KB, patch)
2019-03-15 15:20 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (59.25 KB, patch)
2019-03-15 15:57 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (59.25 KB, patch)
2019-03-15 15:59 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (39.54 KB, patch)
2019-03-15 16:00 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2019-03-14 12:09:26 PDT
Generally, we should be uploading results for all our test suites, to do this, we need a new more general format for uploaded test results. Client-side, that format is managed by webkitpy, so I'm applying the new upload format to that test suite first.
Comment 1 Radar WebKit Bug Importer 2019-03-14 12:11:12 PDT
<rdar://problem/48896182>
Comment 2 Jonathan Bedard 2019-03-14 12:14:45 PDT
Created attachment 364676 [details]
Patch
Comment 3 EWS Watchlist 2019-03-14 12:18:37 PDT
Attachment 364676 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:131:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Lucas Forschler 2019-03-14 12:59:25 PDT
Comment on attachment 364676 [details]
Patch

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

> Tools/Scripts/webkitpy/results/upload.py:36
> +        # These are order by priority, meaning that a test which both crashes and has

ordered

> Tools/Scripts/webkitpy/results/upload.py:164
> +            log_line(' ' * 4 + 'Failed to upload to {}, results server not online'.format(url))

could there be other connection errors besides server offline. could we fail auth? (is there any auth?)

> Tools/Scripts/webkitpy/results/options.py:1
> +# Copyright (C) 2019 Apple Inc. All rights reserved.

duplicate file?

> Tools/Scripts/webkitpy/results/upload.py:1
> +# Copyright (C) 2019 Apple Inc. All rights reserved.

why is this file in the diff twice?

> Tools/Scripts/webkitpy/test/main.py:243
> +        was_upload_successful = True

if feels weird to prime this to True before we upload... I think it should be set True only after upload is finished and we know it was success.

> Tools/Scripts/webkitpy/test/main.py:279
> +                was_upload_successful &= upload.upload(url, log_line=self.printer.meter.writeln)

we set the value for was_upload_successful here... but it's inside an if statement. ( if self._options.report_urls: )
if we never enter this block of code, we'll continue below with was_upload_successful = True (primed from above here)
Comment 5 Jonathan Bedard 2019-03-14 13:04:22 PDT
Comment on attachment 364676 [details]
Patch

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

>> Tools/Scripts/webkitpy/results/upload.py:164
>> +            log_line(' ' * 4 + 'Failed to upload to {}, results server not online'.format(url))
> 
> could there be other connection errors besides server offline. could we fail auth? (is there any auth?)

Yes, there could....although, I would hope those would be returned via a non-200 status code, in that case we should have an error message encoded in json to print (which we try and do a few lines below this)

>> Tools/Scripts/webkitpy/test/main.py:279
>> +                was_upload_successful &= upload.upload(url, log_line=self.printer.meter.writeln)
> 
> we set the value for was_upload_successful here... but it's inside an if statement. ( if self._options.report_urls: )
> if we never enter this block of code, we'll continue below with was_upload_successful = True (primed from above here)

I did this on purpose, and it probably reflects a problem in the variable name...we still want to exit successfully if no upload urls are specified, and I wanted to avoid another if statement when returning status.

Maybe something like 'failed_uploads' as an integer that we increment if uploads fail would be better?
Comment 6 Jonathan Bedard 2019-03-14 17:12:36 PDT
Created attachment 364723 [details]
Patch
Comment 7 EWS Watchlist 2019-03-14 17:15:38 PDT
Attachment 364723 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:131:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Jonathan Bedard 2019-03-14 17:27:57 PDT
Created attachment 364729 [details]
Patch
Comment 9 Jonathan Bedard 2019-03-14 17:28:41 PDT
The fact that some files are in the diff twice is an unfortunate side-effect of svn-create-patch.
Comment 10 EWS Watchlist 2019-03-14 17:30:26 PDT
Attachment 364729 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:131:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Jonathan Bedard 2019-03-15 08:13:13 PDT
Created attachment 364798 [details]
Patch
Comment 12 EWS Watchlist 2019-03-15 08:15:51 PDT
Attachment 364798 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Jonathan Bedard 2019-03-15 08:31:08 PDT
Created attachment 364800 [details]
Patch
Comment 14 EWS Watchlist 2019-03-15 08:33:22 PDT
Attachment 364800 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Jonathan Bedard 2019-03-15 08:53:23 PDT
Created attachment 364801 [details]
Patch
Comment 16 EWS Watchlist 2019-03-15 08:55:51 PDT
Attachment 364801 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Jonathan Bedard 2019-03-15 08:58:43 PDT
Created attachment 364803 [details]
Patch
Comment 18 EWS Watchlist 2019-03-15 09:01:03 PDT
Attachment 364803 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Jonathan Bedard 2019-03-15 09:16:30 PDT
Created attachment 364805 [details]
Patch
Comment 20 EWS Watchlist 2019-03-15 09:18:34 PDT
Attachment 364805 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Jonathan Bedard 2019-03-15 10:46:08 PDT
Created attachment 364810 [details]
Patch
Comment 22 EWS Watchlist 2019-03-15 10:48:45 PDT
Attachment 364810 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Jonathan Bedard 2019-03-15 11:10:38 PDT
Created attachment 364814 [details]
Patch
Comment 24 EWS Watchlist 2019-03-15 11:14:36 PDT
Attachment 364814 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Jonathan Bedard 2019-03-15 11:29:48 PDT
Created attachment 364816 [details]
Patch
Comment 26 EWS Watchlist 2019-03-15 11:32:48 PDT
Attachment 364816 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:133:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Dean Johnson 2019-03-15 14:27:58 PDT
Comment on attachment 364816 [details]
Patch

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

Looks mostly good; unofficial r+ w/ comments.

> Tools/Scripts/webkitpy/results/upload.py:29
> +import platform as platform_

Can you rename this to host_platform instead?

> Tools/Scripts/webkitpy/results/upload.py:32
> +class Upload(object):

The entire Upload class feels a bit messy with the multiple classes defined within it and __init__ being defined about halfway through... maybe it'd be better to pull some of the classes out to the top level since they're already part of the `upload` module?

> Tools/Scripts/webkitpy/results/upload.py:64
> +        ):

For any values which *must* be provided, you may want to consider using positional arguments instead of keyword arguments.

> Tools/Scripts/webkitpy/results/upload.py:75
> +                raise ValueError('{} must be defined in the configuration'.format(key))

Probably worth having a custom exception here.

> Tools/Scripts/webkitpy/results/upload.py:78
> +                config[key] = value

It may be cleaner to do something like:
optional_data = dict(version_name=version_name, model=model, style=style, flavor=flavor, sdk=sdk)
config.update({name: val for val in optiional_data.iteritems() if val is not None})

> Tools/Scripts/webkitpy/results/upload.py:82
> +    def create_commit(repository_id=None, id=None, branch=None):

Ditto to positional vs keyword arguments.

> Tools/Scripts/webkitpy/results/upload.py:97
> +        if options:

It might be more clear to do an early return like:
if not options:
  return result

for element in Upload.BUILDBOT_DETAILS:
  ...
return result

> Tools/Scripts/webkitpy/results/upload.py:98
> +            for element in Upload.BUILDBOT_DETAILS:

Maybe it would be better to put the buildbot-specific logic in its own function, and have that wrap create_details which could be more generalized?

> Tools/Scripts/webkitpy/results/upload.py:107
> +        for key, value in dict(start_time=start_time, end_time=end_time, tests_skipped=tests_skipped).iteritems():

Ditto to positional arguments.

> Tools/Scripts/webkitpy/results/upload.py:117
> +        for key, value in dict(expected=expected, actual=actual, log=log).iteritems():

Ditto to positional arguments.

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

It may be more clear to define log_line as log_line_func.

> Tools/Scripts/webkitpy/results/upload_unittest.py:58
> +        else:

Nit: `else` is redundant.

> Tools/Scripts/webkitpy/results/upload_unittest.py:61
> +    def test_encoding(self):

This is a really nice integration test for Upload!

> Tools/Scripts/webkitpy/results/upload_unittest.py:129
> +        def raise_exception(url, data):

Nit: Might be more clear to define this as part of UploadTest as `raise_requests_ConnectionError`.

> Tools/Scripts/webkitpy/results/upload_unittest.py:135
> +        with mock.patch('requests.post', new=lambda url, data: self.MockResponse(

This broke my brain a little bit.. maybe it would be more clear to define `data: self.MockResponse...` before the `with` block as to keep everything on the same line?

> Tools/Scripts/webkitpy/test/main.py:129
> +        for group_name, group_options in {'Upload Options': upload_options(), 'Configuration Options': [

I think this is a bit of a regression in readability for this area of code. I think it may be more readable if you define 'Configuration Options' outside of the `for` loop.

> Tools/Scripts/webkitpy/test/main.py:248
> +            results = {test: {} for test in test_runner.tests_run}

You may want to add a comment that says "An empty test dictionary indicates the test run was successful/passed."
Comment 28 Jonathan Bedard 2019-03-15 14:48:58 PDT
Comment on attachment 364816 [details]
Patch

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

>> Tools/Scripts/webkitpy/results/upload.py:32
>> +class Upload(object):
> 
> The entire Upload class feels a bit messy with the multiple classes defined within it and __init__ being defined about halfway through... maybe it'd be better to pull some of the classes out to the top level since they're already part of the `upload` module?

The only contained class in 'Expectations'....the rest are static functions. Would you prefer the __init__ method above the static functions?

>> Tools/Scripts/webkitpy/results/upload.py:75
>> +                raise ValueError('{} must be defined in the configuration'.format(key))
> 
> Probably worth having a custom exception here.

If we're going to use positional arguments to enforce values which must be provided, I suppose we can just eliminate this exception entirely.

>> Tools/Scripts/webkitpy/results/upload.py:98
>> +            for element in Upload.BUILDBOT_DETAILS:
> 
> Maybe it would be better to put the buildbot-specific logic in its own function, and have that wrap create_details which could be more generalized?

I might be in favor of that in the future, if we had something other than buildbot details. For now, it seems like extra code that doesn't do much to increase clarity.
Comment 29 Jonathan Bedard 2019-03-15 15:11:28 PDT
Comment on attachment 364816 [details]
Patch

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

>>> Tools/Scripts/webkitpy/results/upload.py:32
>>> +class Upload(object):
>> 
>> The entire Upload class feels a bit messy with the multiple classes defined within it and __init__ being defined about halfway through... maybe it'd be better to pull some of the classes out to the top level since they're already part of the `upload` module?
> 
> The only contained class in 'Expectations'....the rest are static functions. Would you prefer the __init__ method above the static functions?

Talked to Dean about this, I forgot about the Encoder class contained in this. I'm going to move up the encoder to be with Expectations for consistency.
Comment 30 Jonathan Bedard 2019-03-15 15:20:44 PDT
Created attachment 364856 [details]
Patch
Comment 31 Aakash Jain 2019-03-15 15:22:05 PDT
Comment on attachment 364856 [details]
Patch

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

> Tools/Scripts/webkitpy/results/upload.py:34
> +    BUILDBOT_DETAILS = ['buildbot-master', 'builder-name', 'build-number', 'buildbot-worker']

Why doesn't it have builder-id? how will you construct the url back to the build?
Comment 32 Jonathan Bedard 2019-03-15 15:34:57 PDT
Comment on attachment 364856 [details]
Patch

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

>> Tools/Scripts/webkitpy/results/upload.py:34
>> +    BUILDBOT_DETAILS = ['buildbot-master', 'builder-name', 'build-number', 'buildbot-worker']
> 
> Why doesn't it have builder-id? how will you construct the url back to the build?

I was going to have the back end do this with <{host}/api/v2/builders/{name}> during post-processing.
Comment 33 Aakash Jain 2019-03-15 15:38:57 PDT
Comment on attachment 364856 [details]
Patch

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

> Tools/Scripts/webkitpy/results/upload.py:56
> +            if isinstance(obj, Upload):

It's a large block of code inside if. It might be better to do an early return. Something like:

if not isinstance(obj, Upload):
    return super(Upload.Encoder, self).default(obj)
...

> Tools/Scripts/webkitpy/results/upload.py:65
> +                    raise ValueError('All buildbot details must be defined for upload')

Can we make this error message more informative by printing exactly which buildbot detail is missing?

> Tools/Scripts/webkitpy/results/upload.py:135
> +            value = getattr(options, element.replace('-', '_'), None)

why are we replacing '-' with '_'?

> Tools/Scripts/webkitpy/results/upload.py:156
> +    def __init__(self, suite=None, configuration=None, commits=[], timestamp=None, details=None, run_stats=None, results={}):

any specific reason to define __init__ so below in the class (instead of being the first method)?
Comment 34 Jonathan Bedard 2019-03-15 15:55:46 PDT
Comment on attachment 364856 [details]
Patch

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

>> Tools/Scripts/webkitpy/results/upload.py:135
>> +            value = getattr(options, element.replace('-', '_'), None)
> 
> why are we replacing '-' with '_'?

Because ArgParse and OptParse both turn args like 'argument-details' into variables like 'argument_details'

>> Tools/Scripts/webkitpy/results/upload.py:156
>> +    def __init__(self, suite=None, configuration=None, commits=[], timestamp=None, details=None, run_stats=None, results={}):
> 
> any specific reason to define __init__ so below in the class (instead of being the first method)?

I thought it was more clear below the @staticmethods, you and Dean seem to agree it should be the first method, moving it.
Comment 35 Jonathan Bedard 2019-03-15 15:57:00 PDT
Created attachment 364863 [details]
Patch
Comment 36 Jonathan Bedard 2019-03-15 15:59:19 PDT
Created attachment 364866 [details]
Patch
Comment 37 Jonathan Bedard 2019-03-15 16:00:51 PDT
Created attachment 364868 [details]
Patch
Comment 38 EWS Watchlist 2019-03-15 16:06:46 PDT
Attachment 364868 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/results/upload.py:55:  [Upload.Encoder.default] An attribute affected in json.encoder line 162 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 WebKit Commit Bot 2019-03-15 19:49:02 PDT
Comment on attachment 364868 [details]
Patch

Clearing flags on attachment: 364868

Committed r243030: <https://trac.webkit.org/changeset/243030>
Comment 40 WebKit Commit Bot 2019-03-15 19:49:05 PDT
All reviewed patches have been landed.  Closing bug.