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
Patch (20.74 KB, patch)
2019-03-27 17:03 PDT, Jonathan Bedard
no flags
Patch (20.81 KB, patch)
2019-04-01 16:09 PDT, Jonathan Bedard
no flags
Patch for landing (20.75 KB, patch)
2019-04-01 16:58 PDT, Jonathan Bedard
no flags
Patch for landing (20.75 KB, patch)
2019-04-01 19:17 PDT, Jonathan Bedard
no flags
Radar WebKit Bug Importer
Comment 1 2019-03-27 15:38:56 PDT
Jonathan Bedard
Comment 2 2019-03-27 15:43:24 PDT
Jonathan Bedard
Comment 3 2019-03-27 17:03:07 PDT
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
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.