EWS creates 0 byte sized log files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Lucas Forschler]

Ever since r138264, EWS creates 0 length log files.
The only logs which have content, are those with errors.

It is strange to see a directory full of empty files.  We should not be creating them unless something is to be put in them.  
We might not be able to detect this until the run is over... at which we could check for 0 length files, and prune them.

[Eric Seidel (no email)]

They should be smaller, but not zero sized. :(

We can revert the change.

[László Langó]

Created attachment 216578 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 216578 [details]
Patch

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

> Tools/ChangeLog:13
> +
> +        * Scripts/webkitpy/tool/bot/queueengine.py:
> +        (QueueEngine.run):
> +        (QueueEngine._open_work_log):
> +        (QueueEngine._ensure_work_log_closed):
> +        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
> +        (AbstractEarlyWarningSystem.review_patch):

I'd like to see some description as to why this bug manifested and how you fixed it.
There is a lot of code change here, and the correctness of code change is not self-evident.

[László Langó]

(In reply to comment #3)
> (From update of attachment 216578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216578&action=review
> 
> > Tools/ChangeLog:13
> > +
> > +        * Scripts/webkitpy/tool/bot/queueengine.py:
> > +        (QueueEngine.run):
> > +        (QueueEngine._open_work_log):
> > +        (QueueEngine._ensure_work_log_closed):
> > +        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
> > +        (AbstractEarlyWarningSystem.review_patch):
> 
> I'd like to see some description as to why this bug manifested and how you fixed it.
> There is a lot of code change here, and the correctness of code change is not self-evident.

There was a modification in r138264, that tried to make less log, because some of the messeges were duplicated. After this the EWS created the log file (with the same name as the bugID) but doesn't write anything into it, even if there were errors during the build. So i managed this. Now it will create the log file only if there is an error, and wont create if dont want to write anything to it.

The main problem was that, the ScriptError was not raised and the QueueEngine never get that. Now it gets the error and handles it.

[Ryosuke Niwa]

Please add per-function/code change comment as to how you fixed that problem.

[László Langó]

Created attachment 217559 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 217559 [details]
Patch

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

> Tools/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description but after the bug url.

> Tools/ChangeLog:15
> +        (QueueEngine.run): Open the log file only if a ScriptError was raised.

Why?

> Tools/ChangeLog:16
> +        (QueueEngine._open_work_log): Does not need to tee STDOUT to log file anymore.

Why?

> Tools/ChangeLog:17
> +        (QueueEngine._ensure_work_log_closed): Close the logfile.

Why?

> Tools/ChangeLog:19
> +        (AbstractEarlyWarningSystem.review_patch): Raise again the captured ScriptError.

Why?

[Ryosuke Niwa]

The current change log comments simply repeats the code change.  I'm looking for is an explanation as to why each code change is made.

[László Langó]

Created attachment 217684 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 217684 [details]
Patch

Thanks.

[WebKit Commit Bot]

Comment on attachment 217684 [details]
Patch

Clearing flags on attachment: 217684

Committed r159690: <http://trac.webkit.org/changeset/159690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124782

[Alexey Proskuryakov]

This broke webkitpy tests, rolling out. It's not obvious whether these just need an update of results.

http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20(Tests)/builds/15098/steps/webkitpy-test/logs/stdio

[László Langó]

Created attachment 217777 [details]
Patch

[László Langó]

(In reply to comment #14)
> This broke webkitpy tests, rolling out. It's not obvious whether these just need an update of results.
> 
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20(Tests)/builds/15098/steps/webkitpy-test/logs/stdio

Unfortunately i forgot to update the unit test. It really was not obvious, but this change is needed, because of the patch. I wrote some explanation into the Change log too.

[WebKit Commit Bot]

Comment on attachment 217777 [details]
Patch

Clearing flags on attachment: 217777

Committed r159749: <http://trac.webkit.org/changeset/159749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.