169449 2017-03-09 15:06:45 -0800 run-bindings-tests is not aware of specific failures when IDL generation fails 2017-03-14 20:30:11 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 1 webkit webkit-unassigned ap cdumez darin lforschler sam webkit oldest_to_newest 1285631 0 303997 webkit 2017-03-09 15:06:45 -0800 Created attachment 303997 Output of run-bindings-tests The test output (attached) for the below sample change does not point to any specific failure; it just says "some tests fail". It also includes a stack trace from the parser as part of the stdout. diff --git a/Source/WebCore/bindings/scripts/test/TestMapLike.idl b/Source/WebCore/bindings/scripts/test/TestMapLike.idl index fcf1fbf..3d8d4c9 100644 --- a/Source/WebCore/bindings/scripts/test/TestMapLike.idl +++ b/Source/WebCore/bindings/scripts/test/TestMapLike.idl @@ -23,5 +23,5 @@ * THE POSSIBILITY OF SUCH DAMAGE. */ interface MapLike { - maplike<DOMString, DOMString>; + maplike<DOMString>; }; 1286919 1 darin 2017-03-12 11:17:23 -0700 It's not true that the test output does not point to a specific failure. While the syntax is messy, the information is here: Next token should be ,, but > on line 2: maplike<DOMString>; IDLParser.pm:1622 at /Volumes/Data/src/OpenSource/Source/WebCore/bindings/scripts/IDLParser.pm line 216. in /Volumes/Data/src/OpenSource/Source/WebCore/bindings/scripts/test/TestMapLike.idl at /Volumes/Data/src/OpenSource/Source/WebCore/bindings/scripts/IDLParser.pm line 216. The message above says the failure is in TestMapLike.idl, on line 2. But of course the way the IDL parser reports this is messy and you found it unnecessarily confusing. We have put very little time and thinking into good error handling for the IDL parser. The code for this is in IDLParser.pm, the assert function and other functions that call this. We can change this around, but when doing so we should keep in mind that the kind of message that most helps someone implementing the IDL parser is not probably the same format useful to someone making changes to IDL files. As far as the run-bindings-tests script itself is concerned, when generate-bindings.pl fails, run-bindings-tests assumes that the error output from the IDL parser is self explanatory. This is distinct from when the failure is due to successful IDL generation with incorrect output. In that case, the bindings test script definitely needs to write something, since otherwise there is no indication of failure. The PASS/FAIL output comes from the code that compares output to expected output; nothing is written at all if the IDL generation fails entirely. The run-bindings-tests code in question is the run_tests function in the BindingsTests class in Tools/Scripts/webkitpy/bindings/main.py and currently the printing of FAIL and PASS is done entirely by the detect_changes function. We can add printing for the case where IDL generation fails here if we like. That's the line where it calls "passed = False" after calling generate_from_idl, but does not print anything out. Printing out a line that looks like the FAIL line for when comparison fails would be OK with me, although I would probably not make that change because I don't think writing even more is the best way out of this problem. I see some other problems in the run_tests function. One problem is that if generate_from_idl fails we still call detect_changes. That does not seem right. 1287045 2 ap 2017-03-12 23:30:43 -0700 At least as it stands today, not printing FAIL also means that the failing test name is not in JSON results, so the recently added bindings EWS provides less information about the issue. 1287250 3 webkit 2017-03-13 11:18:35 -0700 (In reply to comment #1) > It's not true that the test output does not point to a specific failure. Ah, I should have said: the information doesn't come from run-bindings-tests. Ideally we'd have information about the specific failures in there (eg. "TestMapLike.idl" specifically, as opposed to "something failed, please manually look at the output"). That would enable EWS to make better decisions about patches that fix some failures on a red tree, for example. In the absence of that, Bindings EWS will have to continue to treat such cases as generic failures as it does currently. (eg. Comment 41 on https://bugs.webkit.org/show_bug.cgi?id=159781 ) (In reply to comment #2) > At least as it stands today, not printing FAIL also means that the failing > test name is not in JSON results, so the recently added bindings EWS > provides less information about the issue. We can add stuff to the JSON results without printing, but we don't have any information currently (beyond the fact that something failed). 1287260 4 webkit 2017-03-13 11:47:07 -0700 This might sound nitpicky, but I don't agree with the title change. The issue here is not what is printed. The issue is that it's not able to --report-- some types of failures precisely, and this is independent of the manner of reporting them (whether json output or stdout). 1287699 5 darin 2017-03-14 11:23:36 -0700 Then retitle it again. Your original report made no mention of the JSON and you cited the text output printed; for that your original title was inaccurate. 1287876 6 webkit 2017-03-14 16:48:00 -0700 Retitling to: run-bindings-tests is not aware of specific failures when IDL generation fails. 1287902 7 darin 2017-03-14 17:26:15 -0700 I don’t understand what the words "not aware of" mean here; the script definitely is aware of the failures. That’s why it says the test failed. I think maybe the issue is that the failing files are not listed in the JSON output? 1287905 8 darin 2017-03-14 17:30:05 -0700 I believe the fix for this is to add something like these lines: if self.json_file_name: self.failures.append("(%s) %s" % (generator, input_file)) They could be added just before "return false" after calling the generate_from_idl function in the run_tests function. 1287958 9 webkit 2017-03-14 20:30:11 -0700 Ah, my bad. I had thought the filename there would be a cpp/h file from the subdirectory, not an IDL file, but I was wrong about that! I must have mixed up the loop in run_tests with the loop in detect_changes. Since we are looping over idl files in run_tests, the file for the current iteration ('input_file') is exactly the information that would help EWS. Thanks! 303997 2017-03-09 15:06:45 -0800 2017-03-09 15:06:45 -0800 Output of run-bindings-tests output.txt text/plain 6024 webkit UEFTUzogKEpTKSBKU1Rlc3RDYWxsYmFja0Z1bmN0aW9uLmNwcApQQVNTOiAoSlMpIEpTVGVzdENh bGxiYWNrRnVuY3Rpb24uaApQQVNTOiAoSlMpIEpTVGVzdENhbGxiYWNrRnVuY3Rpb25XaXRoVHlw ZWRlZnMuY3BwClBBU1M6IChKUykgSlNUZXN0Q2FsbGJhY2tGdW5jdGlvbldpdGhUeXBlZGVmcy5o ClBBU1M6IChKUykgSlNUZXN0Q2FsbGJhY2tJbnRlcmZhY2UuY3BwClBBU1M6IChKUykgSlNUZXN0 Q2FsbGJhY2tJbnRlcmZhY2UuaApQQVNTOiAoSlMpIEpTVGVzdENFUmVhY3Rpb25zLmNwcApQQVNT OiAoSlMpIEpTVGVzdENFUmVhY3Rpb25zLmgKUEFTUzogKEpTKSBKU1Rlc3RDRVJlYWN0aW9uc1N0 cmluZ2lmaWVyLmNwcApQQVNTOiAoSlMpIEpTVGVzdENFUmVhY3Rpb25zU3RyaW5naWZpZXIuaApQ QVNTOiAoSlMpIEpTVGVzdENsYXNzV2l0aEpTQnVpbHRpbkNvbnN0cnVjdG9yLmNwcApQQVNTOiAo SlMpIEpTVGVzdENsYXNzV2l0aEpTQnVpbHRpbkNvbnN0cnVjdG9yLmgKUEFTUzogKEpTKSBKU1Rl c3RDdXN0b21Db25zdHJ1Y3RvcldpdGhOb0ludGVyZmFjZU9iamVjdC5jcHAKUEFTUzogKEpTKSBK U1Rlc3RDdXN0b21Db25zdHJ1Y3RvcldpdGhOb0ludGVyZmFjZU9iamVjdC5oClBBU1M6IChKUykg SlNUZXN0Q3VzdG9tTmFtZWRHZXR0ZXIuY3BwClBBU1M6IChKUykgSlNUZXN0Q3VzdG9tTmFtZWRH ZXR0ZXIuaApQQVNTOiAoSlMpIEpTVGVzdEFjdGl2ZURPTU9iamVjdC5jcHAKUEFTUzogKEpTKSBK U1Rlc3RBY3RpdmVET01PYmplY3QuaApQQVNTOiAoSlMpIEpTVGVzdERPTUpJVC5jcHAKUEFTUzog KEpTKSBKU1Rlc3RET01KSVQuaApQQVNTOiAoSlMpIEpTVGVzdEV2ZW50Q29uc3RydWN0b3IuY3Bw ClBBU1M6IChKUykgSlNUZXN0RXZlbnRDb25zdHJ1Y3Rvci5oClBBU1M6IChKUykgSlNUZXN0RXZl bnRUYXJnZXQuY3BwClBBU1M6IChKUykgSlNUZXN0RXZlbnRUYXJnZXQuaApQQVNTOiAoSlMpIEpT VGVzdEV4Y2VwdGlvbi5jcHAKUEFTUzogKEpTKSBKU1Rlc3RFeGNlcHRpb24uaApQQVNTOiAoSlMp IEpTVGVzdEdlbmVyYXRlSXNSZWFjaGFibGUuY3BwClBBU1M6IChKUykgSlNUZXN0R2VuZXJhdGVJ c1JlYWNoYWJsZS5oClBBU1M6IChKUykgSlNUZXN0R2xvYmFsT2JqZWN0LmNwcApQQVNTOiAoSlMp IEpTVGVzdEdsb2JhbE9iamVjdC5oClBBU1M6IChKUykgSlNUZXN0SW1wbGVtZW50cy5jcHAKUEFT UzogKEpTKSBKU1Rlc3RJbXBsZW1lbnRzLmgKUEFTUzogKEpTKSBKU1Rlc3RJbnRlcmZhY2UuY3Bw ClBBU1M6IChKUykgSlNUZXN0SW50ZXJmYWNlLmgKUEFTUzogKEpTKSBKU1Rlc3RJbnRlcmZhY2VM ZWFkaW5nVW5kZXJzY29yZS5jcHAKUEFTUzogKEpTKSBKU1Rlc3RJbnRlcmZhY2VMZWFkaW5nVW5k ZXJzY29yZS5oClBBU1M6IChKUykgSlNUZXN0SXRlcmFibGUuY3BwClBBU1M6IChKUykgSlNUZXN0 SXRlcmFibGUuaApQQVNTOiAoSlMpIEpTVGVzdEpTQnVpbHRpbkNvbnN0cnVjdG9yLmNwcApQQVNT OiAoSlMpIEpTVGVzdEpTQnVpbHRpbkNvbnN0cnVjdG9yLmgKTmV4dCB0b2tlbiBzaG91bGQgYmUg LCwgYnV0ID4gb24gbGluZSAyOiAgICAgbWFwbGlrZTxET01TdHJpbmc+OyBJRExQYXJzZXIucG06 MTYyMiBhdCAvVm9sdW1lcy9EYXRhL3NyYy9PcGVuU291cmNlL1NvdXJjZS9XZWJDb3JlL2JpbmRp bmdzL3NjcmlwdHMvSURMUGFyc2VyLnBtIGxpbmUgMjE2LgogaW4gL1ZvbHVtZXMvRGF0YS9zcmMv T3BlblNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL3Rlc3QvVGVzdE1hcExp a2UuaWRsIGF0IC9Wb2x1bWVzL0RhdGEvc3JjL09wZW5Tb3VyY2UvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0gbGluZSAyMTYuCiRWQVIxID0gJyBhdCAvVm9sdW1l cy9EYXRhL3NyYy9PcGVuU291cmNlL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvSURM UGFyc2VyLnBtIGxpbmUgMjMwLgoJSURMUGFyc2VyOjphc3NlcnRUb2tlblZhbHVlKFwnSURMUGFy c2VyPUhBU0goMHg3Zjk3YWI4MmQyNDgpXCcsIFwnVG9rZW49SEFTSCgweDdmOTdhYzU5YzViMClc JywgXCcsXCcsIDE2MjIpIGNhbGxlZCBhdCAvVm9sdW1lcy9EYXRhL3NyYy9PcGVuU291cmNlL1Nv dXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvSURMUGFyc2VyLnBtIGxpbmUgMTYyMgoJSURM UGFyc2VyOjpwYXJzZU1hcExpa2VQcm9wZXJ0aWVzKFwnSURMUGFyc2VyPUhBU0goMHg3Zjk3YWI4 MmQyNDgpXCcsIFwnSEFTSCgweDdmOTdhYzU5YzE2MClcJywgMCkgY2FsbGVkIGF0IC9Wb2x1bWVz L0RhdGEvc3JjL09wZW5Tb3VyY2UvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQ YXJzZXIucG0gbGluZSAxNjAzCglJRExQYXJzZXI6OnBhcnNlTWFwTGlrZVJlc3QoXCdJRExQYXJz ZXI9SEFTSCgweDdmOTdhYjgyZDI0OClcJywgXCdIQVNIKDB4N2Y5N2FjNTljMTYwKVwnLCAwKSBj YWxsZWQgYXQgL1ZvbHVtZXMvRGF0YS9zcmMvT3BlblNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5k aW5ncy9zY3JpcHRzL0lETFBhcnNlci5wbSBsaW5lIDE0MzkKCUlETFBhcnNlcjo6cGFyc2VPcGVy YXRpb25Pckl0ZXJhdG9yKFwnSURMUGFyc2VyPUhBU0goMHg3Zjk3YWI4MmQyNDgpXCcsIFwnSEFT SCgweDdmOTdhYzU5YzE2MClcJywgMCkgY2FsbGVkIGF0IC9Wb2x1bWVzL0RhdGEvc3JjL09wZW5T b3VyY2UvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0gbGluZSAx MTczCglJRExQYXJzZXI6OnBhcnNlQXR0cmlidXRlT3JPcGVyYXRpb25Pckl0ZXJhdG9yKFwnSURM UGFyc2VyPUhBU0goMHg3Zjk3YWI4MmQyNDgpXCcsIFwnSEFTSCgweDdmOTdhYzU5YzE2MClcJykg Y2FsbGVkIGF0IC9Wb2x1bWVzL0RhdGEvc3JjL09wZW5Tb3VyY2UvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0gbGluZSA3MzYKCUlETFBhcnNlcjo6cGFyc2VJbnRl cmZhY2VNZW1iZXIoXCdJRExQYXJzZXI9SEFTSCgweDdmOTdhYjgyZDI0OClcJywgXCdIQVNIKDB4 N2Y5N2FjNTljMTYwKVwnKSBjYWxsZWQgYXQgL1ZvbHVtZXMvRGF0YS9zcmMvT3BlblNvdXJjZS9T b3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0lETFBhcnNlci5wbSBsaW5lIDcxNQoJSURM UGFyc2VyOjpwYXJzZUludGVyZmFjZU1lbWJlcnMoXCdJRExQYXJzZXI9SEFTSCgweDdmOTdhYjgy ZDI0OClcJykgY2FsbGVkIGF0IC9Wb2x1bWVzL0RhdGEvc3JjL09wZW5Tb3VyY2UvU291cmNlL1dl YkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0gbGluZSA2NDcKCUlETFBhcnNlcjo6 cGFyc2VJbnRlcmZhY2UoXCdJRExQYXJzZXI9SEFTSCgweDdmOTdhYjgyZDI0OClcJywgXCdIQVNI KDB4N2Y5N2FjNTljMjIwKVwnKSBjYWxsZWQgYXQgL1ZvbHVtZXMvRGF0YS9zcmMvT3BlblNvdXJj ZS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0lETFBhcnNlci5wbSBsaW5lIDYwMgoJ SURMUGFyc2VyOjpwYXJzZUNhbGxiYWNrT3JJbnRlcmZhY2UoXCdJRExQYXJzZXI9SEFTSCgweDdm OTdhYjgyZDI0OClcJywgXCdIQVNIKDB4N2Y5N2FjNTljMjIwKVwnKSBjYWxsZWQgYXQgL1ZvbHVt ZXMvRGF0YS9zcmMvT3BlblNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0lE TFBhcnNlci5wbSBsaW5lIDU2OAoJSURMUGFyc2VyOjpwYXJzZURlZmluaXRpb24oXCdJRExQYXJz ZXI9SEFTSCgweDdmOTdhYjgyZDI0OClcJywgXCdIQVNIKDB4N2Y5N2FjNTljMjIwKVwnKSBjYWxs ZWQgYXQgL1ZvbHVtZXMvRGF0YS9zcmMvT3BlblNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5n cy9zY3JpcHRzL0lETFBhcnNlci5wbSBsaW5lIDQ3NAoJSURMUGFyc2VyOjpwYXJzZURlZmluaXRp b25zKFwnSURMUGFyc2VyPUhBU0goMHg3Zjk3YWI4MmQyNDgpXCcpIGNhbGxlZCBhdCAvVm9sdW1l cy9EYXRhL3NyYy9PcGVuU291cmNlL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvSURM UGFyc2VyLnBtIGxpbmUgMjg0CglldmFsIHsuLi59IGNhbGxlZCBhdCAvVm9sdW1lcy9EYXRhL3Ny Yy9PcGVuU291cmNlL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvSURMUGFyc2VyLnBt IGxpbmUgMjgzCglJRExQYXJzZXI6OlBhcnNlKFwnSURMUGFyc2VyPUhBU0goMHg3Zjk3YWI4MmQy NDgpXCcsIFwnL1ZvbHVtZXMvRGF0YS9zcmMvT3BlblNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5k aW5ncy9zY3JpcHRzLy4uLlwnLCBcJ1RFU1RJTkdfSlNcJywgdW5kZWYpIGNhbGxlZCBhdCBXZWJD b3JlL2JpbmRpbmdzL3NjcmlwdHMvZ2VuZXJhdGUtYmluZGluZ3MucGwgbGluZSAxMjkKJzsKJFZB UjEgPSAnIGF0IC9Wb2x1bWVzL0RhdGEvc3JjL09wZW5Tb3VyY2UvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3Mvc2NyaXB0cy9JRExQYXJzZXIucG0gbGluZSAyOTAuCglJRExQYXJzZXI6OlBhcnNlKFwn SURMUGFyc2VyPUhBU0goMHg3Zjk3YWI4MmQyNDgpXCcsIFwnL1ZvbHVtZXMvRGF0YS9zcmMvT3Bl blNvdXJjZS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzLy4uLlwnLCBcJ1RFU1RJTkdf SlNcJywgdW5kZWYpIGNhbGxlZCBhdCBXZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvZ2VuZXJhdGUt YmluZGluZ3MucGwgbGluZSAxMjkKJzsKClBBU1M6IChKUykgSlNUZXN0TWVkaWFRdWVyeUxpc3RM aXN0ZW5lci5jcHAKUEFTUzogKEpTKSBKU1Rlc3RNZWRpYVF1ZXJ5TGlzdExpc3RlbmVyLmgKUEFT UzogKEpTKSBKU1Rlc3ROYW1lZENvbnN0cnVjdG9yLmNwcApQQVNTOiAoSlMpIEpTVGVzdE5hbWVk Q29uc3RydWN0b3IuaApQQVNTOiAoSlMpIEpTVGVzdE5vZGUuY3BwClBBU1M6IChKUykgSlNUZXN0 Tm9kZS5oClBBU1M6IChKUykgSlNUZXN0T2JqLmNwcApQQVNTOiAoSlMpIEpTVGVzdE9iai5oClBB U1M6IChKUykgSlNUZXN0T3ZlcmxvYWRlZENvbnN0cnVjdG9ycy5jcHAKUEFTUzogKEpTKSBKU1Rl c3RPdmVybG9hZGVkQ29uc3RydWN0b3JzLmgKUEFTUzogKEpTKSBKU1Rlc3RPdmVybG9hZGVkQ29u c3RydWN0b3JzV2l0aFNlcXVlbmNlLmNwcApQQVNTOiAoSlMpIEpTVGVzdE92ZXJsb2FkZWRDb25z dHJ1Y3RvcnNXaXRoU2VxdWVuY2UuaApQQVNTOiAoSlMpIEpTVGVzdE92ZXJyaWRlQnVpbHRpbnMu Y3BwClBBU1M6IChKUykgSlNUZXN0T3ZlcnJpZGVCdWlsdGlucy5oClBBU1M6IChKUykgSlNSZWFk T25seU1hcExpa2UuY3BwClBBU1M6IChKUykgSlNSZWFkT25seU1hcExpa2UuaApQQVNTOiAoSlMp IEpTSW50ZXJmYWNlTmFtZS5jcHAKUEFTUzogKEpTKSBKU0ludGVyZmFjZU5hbWUuaApQQVNTOiAo SlMpIEpTVGVzdFNlcmlhbGl6YXRpb24uY3BwClBBU1M6IChKUykgSlNUZXN0U2VyaWFsaXphdGlv bi5oClBBU1M6IChKUykgSlNUZXN0U2VyaWFsaXphdGlvbkluaGVyaXQuY3BwClBBU1M6IChKUykg SlNUZXN0U2VyaWFsaXphdGlvbkluaGVyaXQuaApQQVNTOiAoSlMpIEpTVGVzdFNlcmlhbGl6YXRp b25Jbmhlcml0RmluYWwuY3BwClBBU1M6IChKUykgSlNUZXN0U2VyaWFsaXphdGlvbkluaGVyaXRG aW5hbC5oClBBU1M6IChKUykgSlNUZXN0U2VyaWFsaXplZFNjcmlwdFZhbHVlSW50ZXJmYWNlLmNw cApQQVNTOiAoSlMpIEpTVGVzdFNlcmlhbGl6ZWRTY3JpcHRWYWx1ZUludGVyZmFjZS5oClBBU1M6 IChKUykgSlNUZXN0U3RhbmRhbG9uZURpY3Rpb25hcnkuY3BwClBBU1M6IChKUykgSlNUZXN0U3Rh bmRhbG9uZURpY3Rpb25hcnkuaApQQVNTOiAoSlMpIEpTVGVzdFN0YW5kYWxvbmVFbnVtZXJhdGlv bi5jcHAKUEFTUzogKEpTKSBKU1Rlc3RTdGFuZGFsb25lRW51bWVyYXRpb24uaApQQVNTOiAoSlMp IEpTVGVzdFN1cHBsZW1lbnRhbC5jcHAKUEFTUzogKEpTKSBKU1Rlc3RTdXBwbGVtZW50YWwuaApQ QVNTOiAoSlMpIEpTVGVzdFR5cGVkZWZzLmNwcApQQVNTOiAoSlMpIEpTVGVzdFR5cGVkZWZzLmgK ClNvbWUgdGVzdHMgRkFJTCEgKFRvIHVwZGF0ZSB0aGUgcmVmZXJlbmNlIGZpbGVzLCBleGVjdXRl ICJydW4tYmluZGluZ3MtdGVzdHMgLS1yZXNldC1yZXN1bHRzIikK