WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
196323
run-api-tests: Upload test results
https://bugs.webkit.org/show_bug.cgi?id=196323
Summary
run-api-tests: Upload test results
Jonathan Bedard
Reported
2019-03-27 15:36:43 PDT
We need to upload API test results to the results database.
Attachments
Patch
(17.17 KB, patch)
2019-03-27 15:43 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(20.74 KB, patch)
2019-03-27 17:03 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(20.81 KB, patch)
2019-04-01 16:09 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch for landing
(20.75 KB, patch)
2019-04-01 16:58 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch for landing
(20.75 KB, patch)
2019-04-01 19:17 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-03-27 15:38:56 PDT
<
rdar://problem/49356714
>
Jonathan Bedard
Comment 2
2019-03-27 15:43:24 PDT
Created
attachment 366114
[details]
Patch
Jonathan Bedard
Comment 3
2019-03-27 17:03:07 PDT
Created
attachment 366130
[details]
Patch
Lucas Forschler
Comment 4
2019-04-01 14:04:44 PDT
Comment on
attachment 366130
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=366130&action=review
> Tools/Scripts/webkitpy/api_tests/manager.py:214 > + self._stream.writeln('')
is there any reason to have two statements here instead of: self._stream.writeln('Test suite failed\n') ?
> Tools/Scripts/webkitpy/port/base.py:1662 > + repos['webkit'] = up(up(up(up(up(os.path.abspath(__file__))))))
I'm not sure I've seen this done before. I think in other places we use the ../ syntax, but I guess this is ok too!
> Tools/Scripts/webkitpy/port/base.py:1666 > + scm = SCMDetector(self._filesystem, self._executive).detect_scm_system(path)
how many times are we going to run through this loop? This seems like it could be an expensive operation.
Jonathan Bedard
Comment 5
2019-04-01 14:24:21 PDT
Comment on
attachment 366130
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=366130&action=review
>> Tools/Scripts/webkitpy/api_tests/manager.py:214 >> + self._stream.writeln('') > > is there any reason to have two statements here instead of: > self._stream.writeln('Test suite failed\n') ?
Because of this: class MeteredStream(object): ... @staticmethod def _ensure_newline(txt): return txt if txt.endswith('\n') else txt + '\n' .... def writeln(self, txt, now=None, pid=None): self.write(self._ensure_newline(txt), now, pid)
>> Tools/Scripts/webkitpy/port/base.py:1662 >> + repos['webkit'] = up(up(up(up(up(os.path.abspath(__file__)))))) > > I'm not sure I've seen this done before. I think in other places we use the ../ syntax, but I guess this is ok too!
We do this a few other places: Tools/Scripts/webkitpy//common/multiprocessing_bootstrap.py Tools/Scripts/webkitpy//port/config_standalone.py Tools/Scripts/webkitpy//test/main.py I was adapting the code from Tools/Scripts/webkitpy/test/main.py....I can go either way, but that's why I used the 'up(...)' approach.
>> Tools/Scripts/webkitpy/port/base.py:1666 >> + scm = SCMDetector(self._filesystem, self._executive).detect_scm_system(path) > > how many times are we going to run through this loop? This seems like it could be an expensive operation.
Hopefully not many! Yes, this could be an expensive operation (we maybe should memoize this, actually), this loop will be run for as many repositories appleadditions associates with the test run. We will usually run through this loop 1 or 2 times with the current code.
Lucas Forschler
Comment 6
2019-04-01 15:27:25 PDT
Comment on
attachment 366130
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=366130&action=review
>>> Tools/Scripts/webkitpy/api_tests/manager.py:214 >>> + self._stream.writeln('') >> >> is there any reason to have two statements here instead of: >> self._stream.writeln('Test suite failed\n') ? > > Because of this: > > class MeteredStream(object): > ... > @staticmethod > def _ensure_newline(txt): > return txt if txt.endswith('\n') else txt + '\n' > > .... > def writeln(self, txt, now=None, pid=None): > self.write(self._ensure_newline(txt), now, pid)
well... that's interesting...thanks!
>>> Tools/Scripts/webkitpy/port/base.py:1666 >>> + scm = SCMDetector(self._filesystem, self._executive).detect_scm_system(path) >> >> how many times are we going to run through this loop? This seems like it could be an expensive operation. > > Hopefully not many! Yes, this could be an expensive operation (we maybe should memoize this, actually), this loop will be run for as many repositories appleadditions associates with the test run. We will usually run through this loop 1 or 2 times with the current code.
if we only expect 1 or 2 times, that is fine. in the future we may want to consider memorization..
Jonathan Bedard
Comment 7
2019-04-01 16:09:40 PDT
Created
attachment 366436
[details]
Patch
Jonathan Bedard
Comment 8
2019-04-01 16:58:17 PDT
Created
attachment 366443
[details]
Patch for landing
WebKit Commit Bot
Comment 9
2019-04-01 17:01:48 PDT
Comment on
attachment 366443
[details]
Patch for landing Rejecting
attachment 366443
[details]
from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 366443, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Tools/ChangeLog contains OOPS!. Full output:
https://webkit-queues.webkit.org/results/11732990
Jonathan Bedard
Comment 10
2019-04-01 19:17:27 PDT
Created
attachment 366453
[details]
Patch for landing
WebKit Commit Bot
Comment 11
2019-04-01 19:57:11 PDT
Comment on
attachment 366453
[details]
Patch for landing Clearing flags on attachment: 366453 Committed
r243732
: <
https://trac.webkit.org/changeset/243732
>
WebKit Commit Bot
Comment 12
2019-04-01 19:57:12 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