Fix from r27472618
Created attachment 284599 [details] Patch
Note that this is a temporary fix which needs to be applied to prevent clobbering of crash logs on bots. Long term, there is a refactor needed in much of the port-specific code in the testing scripts.
Comment on attachment 284599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284599&action=review mostly looks good to me. > Tools/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=160198 You might consider including the radar number here. e.g.: <rdar://problem/27472618> > Tools/ChangeLog:11 > + (TestResultWriter.output_filename): Only remove file extension if it is clearly a file extension Nit: end the line with a '.' > Tools/Scripts/webkitpy/common/system/crashlogs.py:34 > class CrashLogs(object): should have two blank lines before class. I wonder why style check did not catch this. > LayoutTests/ChangeLog:5 > + Ditto. > LayoutTests/ChangeLog:8 > + * fast/harness/results.html: Only remove file extension if it is clearly a file extension Ditto. > LayoutTests/fast/harness/results.html:275 > + if (splitExtension(test)[1].length > 5) This logic doesn't match with corresponding python code. splitExtension() returns the exact number of characters in extension, while fs.splitext() in python returns one extra character (counting '.'). For e.g. for "test.html" splitExtension(test)[1].length returns 4, and len(fs.splitext(output_filename)[1]) returns 5.
Created attachment 284614 [details] Patch
Comment on attachment 284614 [details] Patch looks good to me. I probably can't r+, so Alexey or someone has to approve it.
Comment on attachment 284614 [details] Patch Rejecting attachment 284614 [details] from review queue. aakash_jain@apple.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 284614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284614&action=review Looks good to me. > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:93 > + if len(fs.splitext(output_filename)[1]) - 1 > 5: > + return output_filename + modifier Shouldn't we check the extension for test name, not the full path? Could you please add a comment explaining why this is needed and why it's temporary? Something like: # self._test_name can contain either a test name, or <...> (e.g. test-name.txt or com.apple.WebKit.WebContent.Development.123). # FIXME: Refactor to avoid using this variable inconsistently.
Comment on attachment 284614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284614&action=review > LayoutTests/fast/harness/results.html:276 > + if (splitExtension(test)[1].length > 5) > + return test; Also both places needs to be cross-referenced in comments, so that we don't forget to update them at the same time.
Created attachment 284655 [details] Patch
Comment on attachment 284655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284655&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:94 > + # FIXME: Refactor to avoid variable inconsistency. I'm not sure if "variable inconsistency" explains the issue. I'd say something like "FIXME: Refactor to avoid using self._test_name for both test names and process names."
Created attachment 284810 [details] Patch
Comment on attachment 284810 [details] Patch Clearing flags on attachment: 284810 Committed r203902: <http://trac.webkit.org/changeset/203902>
All reviewed patches have been landed. Closing bug.