new-run-webkit-tests does not report stderr output. This could make it slightly harder to diagnose failures.
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.
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.
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?
*** Bug 37422 has been marked as a duplicate of this bug. ***
(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.
Created attachment 98687 [details] Work in progress
Created attachment 98690 [details] Patch
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.
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.
(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?
Thanks! I think I've got at least a minimal test working.
Created attachment 98779 [details] Patch
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.
> 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.
unexpected_results.json
(In reply to comment #15) > unexpected_results.json Ah. True.
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.
Comment on attachment 98779 [details] Patch LGTM.
> 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.
Comment on attachment 98779 [details] Patch Clearing flags on attachment: 98779 Committed r89854: <http://trac.webkit.org/changeset/89854>
All reviewed patches have been landed. Closing bug.
(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.
> 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. :-/
(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?
(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.