Bug 90872 – nrwt crashes saving the output for a platform-specific expected test reference

• Component: New Bugs
• Status: RESOLVED FIXED

Ensure that test_result_writer.copy_file creates the correct directories

Created attachment 151442 [details]
Patch

[Hayato Ito]

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?

Created attachment 151649 [details]
Patch

(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.

[Adam Barth]

@ojan or @dpranke: Thoughts on this patch?

[Ojan Vafai]

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.

[Dirk Pranke]

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

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')

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 :-/

[Dirk Pranke]

(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.

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 151803 [details]
Patch

[Dirk Pranke]

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

[Ojan Vafai]

Comment on attachment 151803 [details]
Patch

Much simpler IMO.

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 151810 [details]
update ChangeLog

(In reply to comment #19)
> Created an attachment (id=151810) [details]
> update ChangeLog

looks good, thanks

[Ryosuke Niwa]

Comment on attachment 151810 [details]
update ChangeLog

The change looks reasonable to me.

[Dirk Pranke]

Committed r122505: <http://trac.webkit.org/changeset/122505>