Bug 37739 – new-run-webkit-tests does not report stderr output

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Maciej Stachowiak]

new-run-webkit-tests does not report stderr output. This could make it slightly harder to diagnose failures.

[Dirk Pranke]

Yup. Note that the stderr output does get logged to the --verbose log (which has been sufficient for the Chromium tree), but I have no argument against adding this feature for better old-run-webkit-tests compatibility.

[Eric Seidel (no email)]

old-run-webkit-tests keeps  a per-test stderr log.  We mostly ignore this log (a zillion tests print to stderr which shouldn't really).  However it's very useful for diagnosing failures (when lower system libraries log things).  Qt and Gtk use it for printing stack traces and all webkit versions log ASSERT failures there.

[Ojan Vafai]

TestShell has stderr output going into the test results. Personally, I think this is better since it means that stderr output doesn't get ignored. Maybe DRT should do the same?

[Ojan Vafai]

*** Bug 37422 has been marked as a duplicate of this bug. ***

[Maciej Stachowiak]

(In reply to comment #3)
> TestShell has stderr output going into the test results. Personally, I think this is better since it means that stderr output doesn't get ignored. Maybe DRT should do the same?

We used to do this, but sometimes there are random changes in std err that we can't easily control, for example due to logging by new versions of Mac OS X system frameworks.

Nontheless, when a test does fail, stderr output is often useful to diagnose what happened.

[Adam Barth]

Created attachment 98687 [details]
Work in progress

[Adam Barth]

Created attachment 98690 [details]
Patch

[Adam Barth]

I don't know how to wrote a test for this patch.  If someone would be willing to give me some pointers, I would very much like to write a test.

[Eric Seidel (no email)]

Comment on attachment 98690 [details]
Patch

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

I suspect it would be possible with on of the integration tests to test this.  There must be one which writes to a mock file system?

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:282
>      def _compare_output_with_reference(self, driver_output1, driver_output2):

I suspect this is an easy function to test.

[Dirk Pranke]

(In reply to comment #8)
> I don't know how to wrote a test for this patch.  If someone would be willing to give me some pointers, I would very much like to write a test.

There's perhaps three things to verify. The first is that the -stderr.txt file does get written to the output directory. The first is already being done (see test_stderr_is_saved() in run_webkit_tests_integrationtest).

The second is that some indicator was stuffed into the results.json file. I'd probably modify the above test to verify that as well (you'd have to read and take apart the results.json file).

The third is to verify that the results.html javascript will display the links properly if the entry is in the json file; I don't really know how we test that stuff, but it probably won't work right with the mock filesystem. Maybe Ojan is already testing this elsewhere?

[Adam Barth]

Thanks!  I think I've got at least a minimal test working.

[Adam Barth]

Created attachment 98779 [details]
Patch

[Dirk Pranke]

Comment on attachment 98779 [details]
Patch

Does the crash actually affect any of the behavior? I would think it wouldn't, and you could reuse/modify the test I mentioned rather than adding a new test file.

[Adam Barth]

> Does the crash actually affect any of the behavior? I would think it wouldn't, and you could reuse/modify the test I mentioned rather than adding a new test file.

My understanding is that I need an unexpected failure for the test to show up in unexpected.json.

[Adam Barth]

unexpected_results.json

[Dirk Pranke]

(In reply to comment #15)
> unexpected_results.json

Ah. True.

[Dirk Pranke]

Comment on attachment 98779 [details]
Patch

I suppose technically you might want to verify that the stderr is showing up in full_results.json as well, but I'm not actually sure if we care about that.

I had kinda lost track of how many different json files we were saving and still using.

[Eric Seidel (no email)]

Comment on attachment 98779 [details]
Patch

LGTM.

[Adam Barth]

> I suppose technically you might want to verify that the stderr is showing up in full_results.json as well, but I'm not actually sure if we care about that.
> 
> I had kinda lost track of how many different json files we were saving and still using.

Yeah, the results blob of data is somewhat out of control.  That seems like something to worry about once we've got the blocking issues fixed because it requires someone to wrap their mind around the global problem.

[Adam Barth]

Comment on attachment 98779 [details]
Patch

Clearing flags on attachment: 98779

Committed r89854: <http://trac.webkit.org/changeset/89854>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

(In reply to comment #19)
> > I suppose technically you might want to verify that the stderr is showing up in full_results.json as well, but I'm not actually sure if we care about that.
> > 
> > I had kinda lost track of how many different json files we were saving and still using.
> 
> Yeah, the results blob of data is somewhat out of control.  That seems like something to worry about once we've got the blocking issues fixed because it requires someone to wrap their mind around the global problem.

FWIW, I've been trying to get us to get rid of unexpected_results.json and to only have full_results.json and incremental_results.json, both of which we need. unexpected_results.json is a subset of the data in full_results.json. There's no longer any benefit to having the former.

[Adam Barth]

> FWIW, I've been trying to get us to get rid of unexpected_results.json and to only have full_results.json and incremental_results.json, both of which we need. unexpected_results.json is a subset of the data in full_results.json. There's no longer any benefit to having the former.

Sorry if I've made your job slightly harder.  :-/

[Dirk Pranke]

(In reply to comment #22)
> (In reply to comment #19)
> > > I suppose technically you might want to verify that the stderr is showing up in full_results.json as well, but I'm not actually sure if we care about that.
> > > 
> > > I had kinda lost track of how many different json files we were saving and still using.
> > 
> > Yeah, the results blob of data is somewhat out of control.  That seems like something to worry about once we've got the blocking issues fixed because it requires someone to wrap their mind around the global problem.
> 
> FWIW, I've been trying to get us to get rid of unexpected_results.json and to only have full_results.json and incremental_results.json, both of which we need. unexpected_results.json is a subset of the data in full_results.json. There's no longer any benefit to having the former.

That was what I was thinking, as well. It should be trivial to change the test to pull the data from full_results.json (and then we wouldn't need the unexpected result, either).

Ojan, we also have the test_times.json file, though, right?

[Ojan Vafai]

(In reply to comment #24)
> Ojan, we also have the test_times.json file, though, right?

Lol. Yeah. We'll need to keep that one as well.