Bug 195755

<rdar://problem/48896182>

Created attachment 364676 [details] Patch

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

Created attachment 364723 [details] Patch

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.

Created attachment 364729 [details] Patch

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

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.

Created attachment 364798 [details] Patch

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.

Created attachment 364800 [details] Patch

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.

Created attachment 364801 [details] Patch

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.

Created attachment 364803 [details] Patch

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.

Created attachment 364805 [details] Patch

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.

Created attachment 364810 [details] Patch

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.

Created attachment 364814 [details] Patch

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.

Created attachment 364816 [details] Patch

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

Created attachment 364856 [details] Patch

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

Created attachment 364863 [details] Patch

Created attachment 364866 [details] Patch

Created attachment 364868 [details] Patch

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 on attachment 364868 [details] Patch Clearing flags on attachment: 364868 Committed r243030: <https://trac.webkit.org/changeset/243030>

All reviewed patches have been landed. Closing bug.