Bug 169449 – run-bindings-tests is not aware of specific failures when IDL generation fails

• Component: Tools / Tests
• Status: NEW

[Srinivasan Vijayaraghavan]

Created attachment 303997 [details]
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>;
 };

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Srinivasan Vijayaraghavan]

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

[Srinivasan Vijayaraghavan]

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

[Darin Adler]

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.

[Srinivasan Vijayaraghavan]

Retitling to: run-bindings-tests is not aware of specific failures when IDL generation fails.

[Darin Adler]

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?

[Darin Adler]

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.

[Srinivasan Vijayaraghavan]

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!