Bug 43473 – [NRWT] Check the DRT #EOF sign at the end of the output line

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Gabor Rapcsanyi]

The NRWT checks the #EOF sign with a line comparison, but the ORWT just checks the line ending.
After this patch the NRWT behaves the same like ORWT.

[Gabor Rapcsanyi]

Created attachment 63430 [details]
proposed patch

[Csaba Osztrogonác]

LGTM, nice fix. Unfortunately I can't give you an r+.

[Eric Seidel (no email)]

OK.  Why isn't #EOF always on its own line?  That seems like a bug.

[Andras Becsi]

(In reply to comment #3)
> OK.  Why isn't #EOF always on its own line?  That seems like a bug.

IIRC we experienced sporadic timeouts with tests where DRT had some stderr output and the DRT #EOF got sometimes flushed out to stdout before the newline, so #EOF got sometimes to the end of some stderr message.

Gabor, it would be good to know which tests timed out with NRWT, so we could grep the source for the message, whether there is some "\n" missing somewhere.

[Dirk Pranke]

(In reply to comment #4)
> (In reply to comment #3)
> > OK.  Why isn't #EOF always on its own line?  That seems like a bug.
> 
> IIRC we experienced sporadic timeouts with tests where DRT had some stderr output and the DRT #EOF got sometimes flushed out to stdout before the newline, so #EOF got sometimes to the end of some stderr message.
> 
> Gabor, it would be good to know which tests timed out with NRWT, so we could grep the source for the message, whether there is some "\n" missing somewhere.

Given that stderr output goes to a different string, that doesn't seem like it should be the case. The implication is that something is not writing a "\n" to stdout somewhere. I could see this potentially happening in a crash, so it seems plausible.

At any rate, the risk is that the parser ends early on a line in the actual output ending in "#EOF". This seems like a minimal risk and we'll notice it in spurious diffs, so this change LGTM as well, although I'm not a reviewer.

[Gabor Rapcsanyi]

(In reply to comment #4)
> (In reply to comment #3)
> > OK.  Why isn't #EOF always on its own line?  That seems like a bug.
> 
> IIRC we experienced sporadic timeouts with tests where DRT had some stderr output and the DRT #EOF got sometimes flushed out to stdout before the newline, so #EOF got sometimes to the end of some stderr message.
> 
> Gabor, it would be good to know which tests timed out with NRWT, so we could grep the source for the message, whether there is some "\n" missing somewhere.

The plugins/document-open.html test times out with NRWT.
The DRT output for this test:

CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS
ERROR: nil result from [documentElement innerText]#EOF
#EOF
#EOF

[Ojan Vafai]

(In reply to comment #6)
> The plugins/document-open.html test times out with NRWT.

Great! (that you found a test that reliably times out). I agree with Eric though. We should fix DRT to print out error messages correctly. Gabor, would you be willing to look into why we're not getting the #EOF on it's own line here?

[Andras Becsi]

(In reply to comment #7)
> (In reply to comment #6)
> > The plugins/document-open.html test times out with NRWT.
> 
> Great! (that you found a test that reliably times out). I agree with Eric though. We should fix DRT to print out error messages correctly. Gabor, would you be willing to look into why we're not getting the #EOF on it's own line here?

This should fix the timeout for Qt, Gabor, could you check:

--- a/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
+++ b/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
@@ -860,7 +860,7 @@ void DumpRenderTree::dump()
         }
 
     } else
-        printf("ERROR: nil result from %s", methodNameStringForFailedTest(m_controller));
+        printf("ERROR: nil result from %s\n", methodNameStringForFailedTest(m_controller));
 
     // signal end of text block
     fputs("#EOF\n", stdout);

There was a missing "\n" indeed, because methodNameStringForFailedTest returns a string without "\n".

[Gabor Rapcsanyi]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > The plugins/document-open.html test times out with NRWT.
> > 
> > Great! (that you found a test that reliably times out). I agree with Eric though. We should fix DRT to print out error messages correctly. Gabor, would you be willing to look into why we're not getting the #EOF on it's own line here?
> 
> This should fix the timeout for Qt, Gabor, could you check:
> 
> --- a/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
> +++ b/WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
> @@ -860,7 +860,7 @@ void DumpRenderTree::dump()
>          }
> 
>      } else
> -        printf("ERROR: nil result from %s", methodNameStringForFailedTest(m_controller));
> +        printf("ERROR: nil result from %s\n", methodNameStringForFailedTest(m_controller));
> 
>      // signal end of text block
>      fputs("#EOF\n", stdout);
> 
> There was a missing "\n" indeed, because methodNameStringForFailedTest returns a string without "\n".

Oh, you're faster than me :)
I tried this and it's worked for me. But it caused text diff mismatch because the expected output is

  CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS

and the output after this patch is (in ORWT too)

  CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS
  ERROR: nil result from [documentElement innerText]

So if we apply this patch on DRT we should create a Qt specific expected file for this or put this test to skip list.

[Csaba Osztrogonác]

(In reply to comment #9)
> > -        printf("ERROR: nil result from %s", methodNameStringForFailedTest(m_controller));
> > +        printf("ERROR: nil result from %s\n", methodNameStringForFailedTest(m_controller));

> Oh, you're faster than me :)
> I tried this and it's worked for me. But it caused text diff mismatch because the expected output is
> 
>   CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS
> 
> and the output after this patch is (in ORWT too)
> 
>   CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS
>   ERROR: nil result from [documentElement innerText]
> 
> So if we apply this patch on DRT we should create a Qt specific expected file for this or put this test to skip list.

Andras, great catch. Gabor, thanks for testing. 

This DRT fix is absolutely necessary to make Qt DRT better. Could you check other DRT's how they handle "ERROR: nil result from" error message? Do they add \n after the error message? But it differs a little bit from the name of
the bug. :) I think, one of you guys should file an other bug and submit a patch for it. Otherwise LGTM.

Well, let's see this bug and the submitted patch ( https://bugs.webkit.org/attachment.cgi?id=63430 ) : In my opinion it is necessary to make EOF detection same in NRWT as in ORWT. But on second thought I prefer the NRWT version of EOF detection, where #EOF must be on its own line. Could we modify ORWT to do same thing?

[Gabor Rapcsanyi]

Will be fixed in DRT.
New bug: https://bugs.webkit.org/show_bug.cgi?id=46806