Bug 196323 – run-api-tests: Upload test results

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We need to upload API test results to the results database.

[Radar WebKit Bug Importer]

<rdar://problem/49356714>

[Jonathan Bedard]

Created attachment 366114 [details]
Patch

[Jonathan Bedard]

Created attachment 366130 [details]
Patch

[Lucas Forschler]

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

Created attachment 366436 [details]
Patch

[Jonathan Bedard]

Created attachment 366443 [details]
Patch for landing

[WebKit Commit Bot]

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]

Created attachment 366453 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366453 [details]
Patch for landing

Clearing flags on attachment: 366453

Committed r243732: <https://trac.webkit.org/changeset/243732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.