Bug 195755 – webkitpy: Upload test results

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

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/48896182>

[Jonathan Bedard]

Created attachment 364676 [details]
Patch

[EWS Watchlist]

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.

[Lucas Forschler]

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)

[Jonathan Bedard]

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?

[Jonathan Bedard]

Created attachment 364723 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364729 [details]
Patch

[Jonathan Bedard]

The fact that some files are in the diff twice is an unfortunate side-effect of svn-create-patch.

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364798 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364800 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364801 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364803 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364805 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364810 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364814 [details]
Patch

[EWS Watchlist]

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.

[Jonathan Bedard]

Created attachment 364816 [details]
Patch

[EWS Watchlist]

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.

[Dean Johnson]

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."

[Jonathan Bedard]

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.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 364856 [details]
Patch

[Aakash Jain]

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?

[Jonathan Bedard]

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.

[Aakash Jain]

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)?

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 364863 [details]
Patch

[Jonathan Bedard]

Created attachment 364866 [details]
Patch

[Jonathan Bedard]

Created attachment 364868 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 364868 [details]
Patch

Clearing flags on attachment: 364868

Committed r243030: <https://trac.webkit.org/changeset/243030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.