| Summary: | nrwt crashes saving the output for a platform-specific expected test reference | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | jochen | ||||||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, dpranke, hayato, ojan, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
jochen
2012-07-10 05:20:47 PDT
Created attachment 151442 [details]
Patch
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. @ojan or @dpranke: Thoughts on this patch? 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 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 :-/ (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. (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. (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. Created attachment 151803 [details]
Patch
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 ? Comment on attachment 151803 [details]
Patch
Much simpler IMO.
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. Created attachment 151810 [details]
update ChangeLog
(In reply to comment #19) > Created an attachment (id=151810) [details] > update ChangeLog looks good, thanks Comment on attachment 151810 [details]
update ChangeLog
The change looks reasonable to me.
Committed r122505: <http://trac.webkit.org/changeset/122505> |