WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(14)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-03-14 12:11:12 PDT
<
rdar://problem/48896182
>
Jonathan Bedard
Comment 2
2019-03-14 12:14:45 PDT
Created
attachment 364676
[details]
Patch
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
Created
attachment 364723
[details]
Patch
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
Created
attachment 364729
[details]
Patch
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
Created
attachment 364798
[details]
Patch
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
Created
attachment 364800
[details]
Patch
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
Created
attachment 364801
[details]
Patch
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
Created
attachment 364803
[details]
Patch
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
Created
attachment 364805
[details]
Patch
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
Created
attachment 364810
[details]
Patch
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
Created
attachment 364814
[details]
Patch
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
Created
attachment 364816
[details]
Patch
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
Created
attachment 364856
[details]
Patch
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
Created
attachment 364863
[details]
Patch
Jonathan Bedard
Comment 36
2019-03-15 15:59:19 PDT
Created
attachment 364866
[details]
Patch
Jonathan Bedard
Comment 37
2019-03-15 16:00:51 PDT
Created
attachment 364868
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug