Bug 56323 – [Qt] Add a command line option to capture stdout and stderr for DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Laszlo Gombos]

LayoutTests rely on redirecting stdout of the DumpRenderTree process. On Symbian redirecting stdout (and stderr) is quite an "involved" process (see http://wiki.forum.nokia.com/index.php/TSS001020_-_Open_C:_Redirecting_stdin_and_stdout_streams or http://qt.gitorious.org/qt/pages/SymbianFAQ).

It would be beneficial if the DumpRendertree executable for the Qt port would have a command line option to specify an outputfile (and perhaps an error file). This is in preparation for running LayoutTests on Symbian. I would recommend to add this option to all Qt supported platforms to make sure that the code is maintained, even though it seems that this is only useful/needed on Symbian.

[qi]

Created attachment 85807 [details]
patch

Here is the proposal, welcome the comments.

[Siddharth Mathur]

Looks reasonable to me.

[Laszlo Gombos]

Comment on attachment 85807 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85807&action=review

Overall the patch looks great. r- to fix the build break.

> Tools/ChangeLog:8
> +        Provide an option for DumpRenderTree to redirect stdout and stderr.

This line does not seem to add information as it just repeats the bug title. Perhaps you should describe how the patch work in plain English.

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:684
> +        freopen(qPrintable(m_redirectErrorFileName), "w", stderr);

This does not build in my environment (Linux) - not sure why it passed on EWS. Fails with the following error:

DumpRenderTreeQt.cpp: In member function ‘void WebCore::DumpRenderTree::processArgsLine(const QStringList&)’:
DumpRenderTreeQt.cpp:682: error: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result
DumpRenderTreeQt.cpp:684: error: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result

You can perhaps check the return value of the freopen call - if (freopen(qPrintable(m_redirectOutputFileName), "w", stdout) == 0) {..}.

> Tools/DumpRenderTree/qt/main.cpp:155
>          qDebug() << "Or folder containing test files: DumpRenderTree [-v|--pixel-tests] dirpath";

I would prefer to have more error-handling for the command-line. It appears that "DumpRenderTree -o FileName" is crashing (as there is no test specified) without any warning.

[qi]

Created attachment 85927 [details]
patch2

Changed the code based on the comments.

[WebKit Review Bot]

Attachment 85927 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/q..." exit_code: 1

Tools/DumpRenderTree/qt/main.cpp:84:  Extra space for operator  ++;  [whitespace/operators] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[qi]

Created attachment 85928 [details]
patch3

fixed the style issue.

[qi]

Comment on attachment 85928 [details]
patch3

find DumpRenderTree has 2 different way to call, current change may broke the run-webkit-tests.

[qi]

Created attachment 85960 [details]
patch4

minor change.

[Benjamin Poulain]

Comment on attachment 85960 [details]
patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=85960&action=review

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:693
> +    if (!m_redirectOutputFileName.isEmpty()) {
> +        if (!freopen(qPrintable(m_redirectOutputFileName), "w", stdout)) {
> +            qDebug() << "Redirect STDOUT failed.";
> +            return;
> +        }
> +    }
> +    if (!m_redirectErrorFileName.isEmpty()) {
> +        if (!freopen(qPrintable(m_redirectErrorFileName), "w", stderr)) {
> +            qDebug() << "Redirect STDERR failed.";
> +            return;
> +        }
> +    }
> +

This looks like a strange place to do this.

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:109
> +    void setRedirectOutputFileName(const QString s) { m_redirectOutputFileName = s; }
> +    void setRedirectErrorFileName(const QString s) { m_redirectErrorFileName = s; }

This should be ref to const.

> Tools/DumpRenderTree/qt/main.cpp:73
> +    if (++index < arguments.count() && !arguments.at(index).startsWith("-"))

!arguments.at(index).startsWith("-")) -> a filename can start with "-"... :)

The side effect with ++index is a bit ugly, you can do better.

> Tools/DumpRenderTree/qt/main.cpp:87
> +bool validOptions(QStringList& args)
> +{
> +    int count = 0;
> +
> +    for (int i = 1; i < args.size(); ++i)
> +        if (!args.at(i).startsWith('-'))
> +            count++;
> +
> +    return count >= 1;
> +}

I don't follow here, at least the function name could be improved.

> Tools/DumpRenderTree/qt/main.cpp:93
> +    fflush(stderr);

Unecesary.

> Tools/DumpRenderTree/qt/main.cpp:192
> +    int outputIndex = args.indexOf("-o");
> +    if (outputIndex != -1) 
> +        dumper.setRedirectOutputFileName(takeOptionValue(args, outputIndex));
> +
> +    int errorIndex = args.indexOf("-e");
> +    if (errorIndex != -1) 
> +        dumper.setRedirectErrorFileName(takeOptionValue(args, errorIndex));
> +

Maybe use long option name? -o and -e are not very intuitive.

You could use QLatin1String for the literals.

[qi]

(In reply to comment #9)
> (From update of attachment 85960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85960&action=review
> 
> > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:693
> > +    if (!m_redirectOutputFileName.isEmpty()) {
> > +        if (!freopen(qPrintable(m_redirectOutputFileName), "w", stdout)) {
> > +            qDebug() << "Redirect STDOUT failed.";
> > +            return;
> > +        }
> > +    }
> > +    if (!m_redirectErrorFileName.isEmpty()) {
> > +        if (!freopen(qPrintable(m_redirectErrorFileName), "w", stderr)) {
> > +            qDebug() << "Redirect STDERR failed.";
> > +            return;
> > +        }
> > +    }
> > +
> 
> This looks like a strange place to do this.

Maybe I can do this at main.cpp, the reason I put the code here is in main.cpp I have no place to put the "stop freopen" code. I don't think main.cpp can connect to DumpRenderTreeQt's quit()??!! My purpose is to keep the start and stop in the same class.

> 
> > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:109
> > +    void setRedirectOutputFileName(const QString s) { m_redirectOutputFileName = s; }
> > +    void setRedirectErrorFileName(const QString s) { m_redirectErrorFileName = s; }
> 
> This should be ref to const.
> 
> > Tools/DumpRenderTree/qt/main.cpp:73
> > +    if (++index < arguments.count() && !arguments.at(index).startsWith("-"))
> 
> !arguments.at(index).startsWith("-")) -> a filename can start with "-"... :)

really? you mean "-abc.html" is a valid name? I copy this code from other place, actually in the main.cpp, this is a assumption that start with "-" should be a option like "-v".

> 
> The side effect with ++index is a bit ugly, you can do better.
> 
> > Tools/DumpRenderTree/qt/main.cpp:87
> > +bool validOptions(QStringList& args)
> > +{
> > +    int count = 0;
> > +
> > +    for (int i = 1; i < args.size(); ++i)
> > +        if (!args.at(i).startsWith('-'))
> > +            count++;
> > +
> > +    return count >= 1;
> > +}
> 
> I don't follow here, at least the function name could be improved.

DumpRenderTreeQt need at least one test case name (not start with "-"), the code here just check if we do have.

> 
> > Tools/DumpRenderTree/qt/main.cpp:93
> > +    fflush(stderr);
> 
> Unecesary.

I found if I don't put fflush here, some time another stderr message (LEAK) just jump into my two print out, make it looks ugly.

> 
> > Tools/DumpRenderTree/qt/main.cpp:192
> > +    int outputIndex = args.indexOf("-o");
> > +    if (outputIndex != -1) 
> > +        dumper.setRedirectOutputFileName(takeOptionValue(args, outputIndex));
> > +
> > +    int errorIndex = args.indexOf("-e");
> > +    if (errorIndex != -1) 
> > +        dumper.setRedirectErrorFileName(takeOptionValue(args, errorIndex));
> > +
> 
> Maybe use long option name? -o and -e are not very intuitive.
> 
> You could use QLatin1String for the literals.

Maybe you like : -stdout and -stderr

[qi]

Created attachment 86079 [details]
patch5

Change based on discussion with Laszlo and comments from Benjamin.

1. Make the redirect work for standalone mode and "-" mode.
2. Because 1, I move the code for starting redirect into main.cpp.
3. Changed DumpRenderTreeQt.h based on Benjanmin's comments.
4. Based Benjanmin's comments, "-bac.html" is valid test case name. The assumption to get test case name is wrong, I make a function to identify the option name, then all the others will be consider as valid option value.
5. Changed the redirection option name as "--stdout" and "--stderr"

[qi]

Created attachment 86189 [details]
patch6

Do some improvement.

[qi]

Created attachment 86191 [details]
patch6

sorry, upload wrong patch.

[Kenneth Rohde Christiansen]

Comment on attachment 86191 [details]
patch6

View in context: https://bugs.webkit.org/attachment.cgi?id=86191&action=review

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:108
> +    void setRedirectOutputFileName(const QString& s) { m_redirectOutputFileName = s; }

name would be more descriptive than s

Also, please check whether we use Filename or FileName in Qt APIs. I believe we use the former.

> Tools/DumpRenderTree/qt/main.cpp:79
> +// Get the option value and remove the option and value from arguments list

I dont think this comment is needed. The code is very condense and easy to understand and the method name is also quite descriptive

> Tools/DumpRenderTree/qt/main.cpp:201
> +            fprintf(stderr, "Redirect STDOUT failed.");

stdout redirection failed. - Redirection of stdout failed.

> Tools/DumpRenderTree/qt/main.cpp:202
> +            exit(0);

Isn't that an error?

> Tools/DumpRenderTree/qt/main.cpp:228
> +            // Go to read STDIN

Comments like this one are not that useful and might end up bitrotting.

[qi]

Created attachment 86314 [details]
patch7

Change based on comment 14.

I found in qwebpage.h used "FileName", so I didn't change my code (see the first comment is comment 14)

[Laszlo Gombos]

Comment on attachment 86314 [details]
patch7

r=me.

[WebKit Commit Bot]

Comment on attachment 86314 [details]
patch7

Clearing flags on attachment: 86314

Committed r81588: <http://trac.webkit.org/changeset/81588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.