Bug 90872 - nrwt crashes saving the output for a platform-specific expected test reference
Summary: nrwt crashes saving the output for a platform-specific expected test reference
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-10 05:20 PDT by jochen
Modified: 2012-07-12 14:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.61 KB, patch)
2012-07-10 05:23 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2012-07-11 01:58 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (18.18 KB, patch)
2012-07-11 15:31 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update ChangeLog (18.19 KB, patch)
2012-07-11 16:04 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-07-10 05:20:47 PDT
Ensure that test_result_writer.copy_file creates the correct directories
Comment 1 jochen 2012-07-10 05:23:51 PDT
Created attachment 151442 [details]
Patch
Comment 2 Hayato Ito 2012-07-10 16:31:50 PDT
Comment on attachment 151442 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:70
> +        except Exception, e:

Should we catch an exception here?
If a test_function throws Exception, the unit test framework treats that as a failure (or an error) of the test case automatically, doesn't it?
Comment 3 jochen 2012-07-11 01:58:17 PDT
Created attachment 151649 [details]
Patch
Comment 4 jochen 2012-07-11 01:58:43 PDT
(In reply to comment #2)
> (From update of attachment 151442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151442&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:70
> > +        except Exception, e:
> 
> Should we catch an exception here?
> If a test_function throws Exception, the unit test framework treats that as a failure (or an error) of the test case automatically, doesn't it?

Right.
Comment 5 Adam Barth 2012-07-11 11:56:17 PDT
@ojan or @dpranke: Thoughts on this patch?
Comment 6 Ojan Vafai 2012-07-11 12:05:58 PDT
Comment on attachment 151649 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:270
> +        fs.maybe_make_directory(fs.dirname(dst_filepath))

Do we not need this in the other places in this file where we call make_output_directory? Do write_text_file/write_binary_file create the directory for you?

I suppose there's already a test that shows the write_binary_file codepath works correctly.
Comment 7 Dirk Pranke 2012-07-11 12:16:27 PDT
Comment on attachment 151649 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:270
>> +        fs.maybe_make_directory(fs.dirname(dst_filepath))
> 
> Do we not need this in the other places in this file where we call make_output_directory? Do write_text_file/write_binary_file create the directory for you?
> 
> I suppose there's already a test that shows the write_binary_file codepath works correctly.

This is basically duplicating the logic in self._make_output_directory(), except that we're using src_filepath instead of self._test_name). We should just modify _make_output_directory() to take the filename as input.

It's also not clear to me why the output_directory isn't already created, since we tend to create pretty early on in most of the code paths through this class, so I'm concerned that there's some other bug or inconsistency this is papering over. What test was failing because of this? I'd like to see what code was executing ...
Comment 8 jochen 2012-07-11 12:27:05 PDT
I ran across the problem with content_shell. It doesn't reproduce with DRT, as DRT matches the expectation, so no output is written...

Here's the command line:

DISPLAY=:4 new-run-webkit-tests --chromium --debug --clobber-old-results fast/regions/region-style-rule-specificity.html --driver-name=content_shell --additional-drt-flag=--dump-render-tree


and here's the stack trace

    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 297, in run
      self._worker_connection.run_message_loop()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 251, in run_message_loop
      self._broker.run_message_loop(self._run_topic, self._client, delay_secs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 183, in run_message_loop
      self._run_loop(topic_name, client, block=True, delay_secs=delay_secs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 197, in _run_loop
      self._dispatch_message(msg, client)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 206, in _dispatch_message
      message_handler(message.src, *optargs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 320, in handle_test_list
      self.worker.handle('test_list', source, list_name, test_list)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 82, in handle
      self._run_test(test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 106, in _run_test
      result = self.run_test_with_timeout(test_input, test_timeout_sec)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 148, in run_test_with_timeout
      return self._run_test_in_this_thread(test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 233, in _run_test_in_this_thread
      return self.run_single_test(self._driver, test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 237, in run_single_test
      test_input, driver, self._name)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 46, in run_single_test
      return runner.run()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 102, in run
      return self._run_reftest()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 301, in _run_reftest
      test_result_writer.write_test_result(self._filesystem, self._port, self._test_name, test_output, reference_output, test_result.failures)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py", line 77, in write_test_result
      writer.copy_file(failure.reference_filename)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py", line 271, in copy_file
      fs.copyfile(src_filepath, dst_filepath)
    File "/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py", line 76, in copyfile
      shutil.copyfile(source, destination)
    File "/usr/lib/python2.6/shutil.py", line 53, in copyfile
      fdst = open(dst, 'wb')
Exception raised, exiting
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 129, in run
      unexpected_result_count = manager.run()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 910, in run
      interrupted, keyboard_interrupted, thread_timings, test_timings, individual_test_timings = self._run_tests(self._test_files_list, result_summary, int(self._options.child_processes))
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 802, in _run_tests
      manager_connection.run_message_loop(delay_secs=1.0)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 358, in run_message_loop
      self._inline_worker.run()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 454, in run
      self._client.run(self._host)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 306, in run
      self._worker_connection.raise_exception(sys.exc_info())
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 297, in run
      self._worker_connection.run_message_loop()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 251, in run_message_loop
      self._broker.run_message_loop(self._run_topic, self._client, delay_secs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 183, in run_message_loop
      self._run_loop(topic_name, client, block=True, delay_secs=delay_secs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 197, in _run_loop
      self._dispatch_message(msg, client)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 206, in _dispatch_message
      message_handler(message.src, *optargs)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 320, in handle_test_list
      self.worker.handle('test_list', source, list_name, test_list)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 82, in handle
      self._run_test(test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 106, in _run_test
      result = self.run_test_with_timeout(test_input, test_timeout_sec)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 148, in run_test_with_timeout
      return self._run_test_in_this_thread(test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 233, in _run_test_in_this_thread
      return self.run_single_test(self._driver, test_input)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py", line 237, in run_single_test
      test_input, driver, self._name)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 46, in run_single_test
      return runner.run()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 102, in run
      return self._run_reftest()
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py", line 301, in _run_reftest
      test_result_writer.write_test_result(self._filesystem, self._port, self._test_name, test_output, reference_output, test_result.failures)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py", line 77, in write_test_result
      writer.copy_file(failure.reference_filename)
    File "/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py", line 271, in copy_file
      fs.copyfile(src_filepath, dst_filepath)
    File "/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py", line 76, in copyfile
      shutil.copyfile(source, destination)
    File "/usr/lib/python2.6/shutil.py", line 53, in copyfile
      fdst = open(dst, 'wb')
Comment 9 jochen 2012-07-11 12:28:45 PDT
The problem seems to be that the test is not platform specific, but _make_output_dir makes the directory based on the test name. The expectation, however, is platform specific, and there's nothing in the writer that would create the platform specific dir.

Also, the _make_output_dir function is only invoked in a few places, many places directly invoke fs.maybe_make_directoriy :-/
Comment 10 Dirk Pranke 2012-07-11 12:30:32 PDT
(In reply to comment #9)
> The problem seems to be that the test is not platform specific, but _make_output_dir makes the directory based on the test name. The expectation, however, is platform specific, and there's nothing in the writer that would create the platform specific dir.

the files in the results directory are always supposed to be next to the test, not put in a platform-specific directory.
Comment 11 jochen 2012-07-11 12:36:05 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > The problem seems to be that the test is not platform specific, but _make_output_dir makes the directory based on the test name. The expectation, however, is platform specific, and there's nothing in the writer that would create the platform specific dir.
> 
> the files in the results directory are always supposed to be next to the test, not put in a platform-specific directory.

ok - that's however not what the code does. E.g. the results.html will include a to the -expected.html file in the platform directory.
Comment 12 Dirk Pranke 2012-07-11 12:41:29 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > The problem seems to be that the test is not platform specific, but _make_output_dir makes the directory based on the test name. The expectation, however, is platform specific, and there's nothing in the writer that would create the platform specific dir.
> > 
> > the files in the results directory are always supposed to be next to the test, not put in a platform-specific directory.
> 
> ok - that's however not what the code does. E.g. the results.html will include a to the -expected.html file in the platform directory.

Hm. Maybe my memory is bad, then, and it's only the -actual files that are supposed to be next to the tests, although that seems like it would be annoying. At any rate, I'll dig through it shortly so hopefully this  is only a minor delay.
Comment 13 Dirk Pranke 2012-07-11 13:19:14 PDT
(In reply to comment #11)
> ok - that's however not what the code does. E.g. the results.html will include a to the -expected.html file in the platform directory.

I see, this is a problem with reftests; all of the other files are placed next to the test itself. I will fix that and post a patch.
Comment 14 Dirk Pranke 2012-07-11 15:31:09 PDT
Created attachment 151803 [details]
Patch
Comment 15 Dirk Pranke 2012-07-11 15:36:13 PDT
Okay, it turns out that for ref tests we actually left the -expected file in its (relative) original place in the tree, so if we had foo/bar.html and platform/mac/foo/bar-expected.txt, it would get copied next to bar.html, but if we had platform/mac/foo/bar-expected.html, it would get copied to platform/mac/foo.

Except of course, the latter code path didn't actually work since the directory might not exist :)

It seems bad to me to put these files in two different places. Putting the output next to the test is definitely convenient, but I can also see the argument for leaving the output in the original location since it's clearer which file is being compared against. 

The patch I just posted changes where we store reftests so that they are stored next to the test. This makes the code simpler, but does lose some information.

rniwa, hayato, what do you think?

We could also copy the reference output next to the test, but leave the ref_file input in the JSON so at least the information is kept in case we decide to use it somewhere/sometime. It might be nice to also set 'ref_file' for txt and pixel paths in this case.
Comment 16 jochen 2012-07-11 15:36:46 PDT
Comment on attachment 151803 [details]
Patch

Just some nits. I have too little insight into this to actually review the code changes

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

> Tools/ChangeLog:3
> +        Ensure that test_result_writer.copy_file creates the correct directories

now that you actually remove copy_file, this doesn't make sense anymore

> Tools/ChangeLog:10
> +        foo/bar-expected.txt sits alongside foo/bar-expected.html even

just foo/bar.html ?
Comment 17 Ojan Vafai 2012-07-11 15:40:28 PDT
Comment on attachment 151803 [details]
Patch

Much simpler IMO.
Comment 18 Dirk Pranke 2012-07-11 15:53:31 PDT
Comment on attachment 151803 [details]
Patch

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

>> Tools/ChangeLog:3
>> +        Ensure that test_result_writer.copy_file creates the correct directories
> 
> now that you actually remove copy_file, this doesn't make sense anymore

true. will update the bug subject.

>> Tools/ChangeLog:10
>> +        foo/bar-expected.txt sits alongside foo/bar-expected.html even
> 
> just foo/bar.html ?

will fix.
Comment 19 Dirk Pranke 2012-07-11 16:04:05 PDT
Created attachment 151810 [details]
update ChangeLog
Comment 20 jochen 2012-07-12 00:28:00 PDT
(In reply to comment #19)
> Created an attachment (id=151810) [details]
> update ChangeLog

looks good, thanks
Comment 21 Ryosuke Niwa 2012-07-12 13:24:27 PDT
Comment on attachment 151810 [details]
update ChangeLog

The change looks reasonable to me.
Comment 22 Dirk Pranke 2012-07-12 14:22:23 PDT
Committed r122505: <http://trac.webkit.org/changeset/122505>