REGRESSION (r230998): Cannot stream API test output

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

It used to be the case that API tests would stream their output before timing out.  After r230998, this output was cached and only printed after a test timed out.

[Jonathan Bedard]

Created attachment 339025 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 339025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339025&action=review

> Tools/Scripts/webkitpy/api_tests/runner.py:108
> +                        self.printer.writeln(source)

Does this output with timestamps? I really don't want to see that in the streamed results.
Because it would make copying & pasting multiple lines of output hard.
e.g. for dumping render tree, etc...

> Tools/Scripts/webkitpy/api_tests/runner.py:159
> +        status = Runner.STATUS_FAILED

Sounds like this is really STATUS_RUNNING?

> Tools/Scripts/webkitpy/api_tests/runner.py:166
> +            if status != Runner.STATUS_DISABLED:
> +                server_process.start()

Why is this code change needed?

> Tools/Scripts/webkitpy/api_tests/runner.py:177
> +                    self._caller.post('log', None, None, stderr_line[:-1])

Name arguments to which you're specifying None. Otherwise, the reader of this code can't tell what's being set to None.

> Tools/Scripts/webkitpy/api_tests/runner.py:183
> +                        self._caller.post('log', None, None, stdout_line[:-1])

Ditto.

[Ryosuke Niwa]

Comment on attachment 339025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339025&action=review

> Tools/Scripts/webkitpy/api_tests/runner.py:52
> +        self._has_logged_for_test = True

Why this instance variable initially set to True?

[Jonathan Bedard]

Comment on attachment 339025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339025&action=review

>> Tools/Scripts/webkitpy/api_tests/runner.py:52
>> +        self._has_logged_for_test = True
> 
> Why this instance variable initially set to True?

This is basically a cosmetic difference.

False would mean an empty line between the output 'Running tests', true means there is no empty line. I thought that not having the empty line looked better.

>> Tools/Scripts/webkitpy/api_tests/runner.py:108
>> +                        self.printer.writeln(source)
> 
> Does this output with timestamps? I really don't want to see that in the streamed results.
> Because it would make copying & pasting multiple lines of output hard.
> e.g. for dumping render tree, etc...

This would output timestamps in verbose mode, yes.

It sounds like that isn't desirable, I'll change it.

>> Tools/Scripts/webkitpy/api_tests/runner.py:166
>> +                server_process.start()
> 
> Why is this code change needed?

Because we really shouldn't have been starting the process if the test was disabled in the first place.

This code change isn't needed, exactly (the old way did work), but it is better because it doesn't spawn an extra process needlessly.

[Jonathan Bedard]

Created attachment 339123 [details]
Patch

[Jonathan Bedard]

Comment on attachment 339123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339123&action=review

> Tools/Scripts/webkitpy/api_tests/runner.py:100
> +        source = '' if self._num_workers == 1 else source + ' '

I wanted to call out what changed here, base on Ryosuke's request in comment 2: 'Does this output with timestamps? I really don't want to see that in the streamed results.'

The new patch will not print timestamps unless specifically asked to do so. It will also not print the worker information if only one worker is used. This should make logs easy to copy paste.

[Ryosuke Niwa]

Comment on attachment 339123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339123&action=review

> Tools/Scripts/webkitpy/api_tests/runner.py:53
> +        self._has_logged_for_test = True

We should add a comment here saying this would suppress an empty line between "Running tests" and the first test's output.

> Tools/Scripts/webkitpy/layout_tests/views/metered_stream.py:57
> +        self._print_timestamps = verbose if print_timestamps is None else print_timestamps

It's a bit odd that this falls back to verbose but I guess changing it everywhere would be a lot of code changes.

[Jonathan Bedard]

Created attachment 340142 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 340142 [details]
Patch for landing

Clearing flags on attachment: 340142

Committed r231679: <https://trac.webkit.org/changeset/231679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40148803>