RESOLVED FIXED 195755
webkitpy: Upload test results
https://bugs.webkit.org/show_bug.cgi?id=195755
Summary webkitpy: Upload test results
Jonathan Bedard
Reported 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.
Attachments
Patch (36.80 KB, patch)
2019-03-14 12:14 PDT, Jonathan Bedard
no flags
Patch (57.25 KB, patch)
2019-03-14 17:12 PDT, Jonathan Bedard
no flags
Patch (58.84 KB, patch)
2019-03-14 17:27 PDT, Jonathan Bedard
no flags
Patch (58.95 KB, patch)
2019-03-15 08:13 PDT, Jonathan Bedard
no flags
Patch (59.75 KB, patch)
2019-03-15 08:31 PDT, Jonathan Bedard
no flags
Patch (60.09 KB, patch)
2019-03-15 08:53 PDT, Jonathan Bedard
no flags
Patch (60.12 KB, patch)
2019-03-15 08:58 PDT, Jonathan Bedard
no flags
Patch (60.35 KB, patch)
2019-03-15 09:16 PDT, Jonathan Bedard
no flags
Patch (40.51 KB, patch)
2019-03-15 10:46 PDT, Jonathan Bedard
no flags
Patch (40.51 KB, patch)
2019-03-15 11:10 PDT, Jonathan Bedard
no flags
Patch (40.54 KB, patch)
2019-03-15 11:29 PDT, Jonathan Bedard
no flags
Patch (39.49 KB, patch)
2019-03-15 15:20 PDT, Jonathan Bedard
no flags
Patch (59.25 KB, patch)
2019-03-15 15:57 PDT, Jonathan Bedard
no flags
Patch (59.25 KB, patch)
2019-03-15 15:59 PDT, Jonathan Bedard
no flags
Patch (39.54 KB, patch)
2019-03-15 16:00 PDT, Jonathan Bedard
no flags
Radar WebKit Bug Importer
Comment 1 2019-03-14 12:11:12 PDT
Jonathan Bedard
Comment 2 2019-03-14 12:14:45 PDT
EWS Watchlist
Comment 3 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.
Lucas Forschler
Comment 4 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)
Jonathan Bedard
Comment 5 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?
Jonathan Bedard
Comment 6 2019-03-14 17:12:36 PDT
EWS Watchlist
Comment 7 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.
Jonathan Bedard
Comment 8 2019-03-14 17:27:57 PDT
Jonathan Bedard
Comment 9 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.
EWS Watchlist
Comment 10 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.
Jonathan Bedard
Comment 11 2019-03-15 08:13:13 PDT
EWS Watchlist
Comment 12 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.
Jonathan Bedard
Comment 13 2019-03-15 08:31:08 PDT
EWS Watchlist
Comment 14 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.
Jonathan Bedard
Comment 15 2019-03-15 08:53:23 PDT
EWS Watchlist
Comment 16 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.
Jonathan Bedard
Comment 17 2019-03-15 08:58:43 PDT
EWS Watchlist
Comment 18 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.
Jonathan Bedard
Comment 19 2019-03-15 09:16:30 PDT
EWS Watchlist
Comment 20 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.
Jonathan Bedard
Comment 21 2019-03-15 10:46:08 PDT
EWS Watchlist
Comment 22 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.
Jonathan Bedard
Comment 23 2019-03-15 11:10:38 PDT
EWS Watchlist
Comment 24 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.
Jonathan Bedard
Comment 25 2019-03-15 11:29:48 PDT
EWS Watchlist
Comment 26 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.
Dean Johnson
Comment 27 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."
Jonathan Bedard
Comment 28 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.
Jonathan Bedard
Comment 29 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.
Jonathan Bedard
Comment 30 2019-03-15 15:20:44 PDT
Aakash Jain
Comment 31 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?
Jonathan Bedard
Comment 32 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.
Aakash Jain
Comment 33 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)?
Jonathan Bedard
Comment 34 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.
Jonathan Bedard
Comment 35 2019-03-15 15:57:00 PDT
Jonathan Bedard
Comment 36 2019-03-15 15:59:19 PDT
Jonathan Bedard
Comment 37 2019-03-15 16:00:51 PDT
EWS Watchlist
Comment 38 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.
WebKit Commit Bot
Comment 39 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>
WebKit Commit Bot
Comment 40 2019-03-15 19:49:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.