Bug 124703 - Remove the stderr_write attribute from StyleProcessorConfiguration
Summary: Remove the stderr_write attribute from StyleProcessorConfiguration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 124726
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-21 01:12 PST by László Langó
Modified: 2013-12-02 15:49 PST (History)
7 users (show)

See Also:


Attachments
Resolve a FIXME (3.91 KB, patch)
2013-11-21 01:22 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (17.94 KB, patch)
2013-11-22 02:06 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (17.50 KB, patch)
2013-11-22 05:40 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2013-11-22 06:27 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description László Langó 2013-11-21 01:12:23 PST
Remove the stderr_write attribute from this class in checker and replace its use with calls to a logging module logger.
Comment 1 László Langó 2013-11-21 01:22:57 PST
Created attachment 217531 [details]
Resolve a FIXME
Comment 2 Brent Fulgham 2013-11-21 09:16:10 PST
Comment on attachment 217531 [details]
Resolve a FIXME

Seems like all tests pass after this change.  r=me.
Comment 3 WebKit Commit Bot 2013-11-21 09:41:57 PST
Comment on attachment 217531 [details]
Resolve a FIXME

Clearing flags on attachment: 217531

Committed r159633: <http://trac.webkit.org/changeset/159633>
Comment 4 WebKit Commit Bot 2013-11-21 09:41:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Dean Jackson 2013-11-21 10:50:20 PST
This broke a bunch of the webkitpy tests.
Comment 6 WebKit Commit Bot 2013-11-21 10:54:53 PST
Re-opened since this is blocked by bug 124726
Comment 7 Brent Fulgham 2013-11-21 11:01:01 PST
(In reply to comment #2)
> (From update of attachment 217531 [details])
> Seems like all tests pass after this change.  r=me.

Boy was I wrong!  EWS does not run tests, so I'm going to start requiring them for review.
Comment 8 László Langó 2013-11-21 23:05:08 PST
(In reply to comment #7)
> (In reply to comment #2)
> > (From update of attachment 217531 [details] [details])
> > Seems like all tests pass after this change.  r=me.
> 
> Boy was I wrong!  EWS does not run tests, so I'm going to start requiring them for review.

Sorry, i tested it before uploaded and all tests are passed in local. I'll try to figure out what's the problem.
Comment 9 László Langó 2013-11-22 02:06:54 PST
Created attachment 217660 [details]
Patch
Comment 10 Peter Gal 2013-11-22 04:49:28 PST
Comment on attachment 217660 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checker_unittest.py:580
>      def setUp(self):
> -        self._error_messages = []
> -        """The messages written to _mock_stderr_write() of this class."""
> +        LoggingTestCase.setUp(self)

I don't think you need this setUp (and tearDown) method here, because you are explicitly calling the base class' setUp (/tearDown) method. If you don't implement the methods here it'll be inherited, so that's why there is no need for this methods.

Also calling the base class' method this way is not really good, we should use the super() call for such cases (http://docs.python.org/2/library/functions.html#super).
Comment 11 László Langó 2013-11-22 04:58:35 PST
(In reply to comment #10)
> (From update of attachment 217660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217660&action=review
> 
> > Tools/Scripts/webkitpy/style/checker_unittest.py:580
> >      def setUp(self):
> > -        self._error_messages = []
> > -        """The messages written to _mock_stderr_write() of this class."""
> > +        LoggingTestCase.setUp(self)
> 
> I don't think you need this setUp (and tearDown) method here, because you are explicitly calling the base class' setUp (/tearDown) method. If you don't implement the methods here it'll be inherited, so that's why there is no need for this methods.
> 
> Also calling the base class' method this way is not really good, we should use the super() call for such cases (http://docs.python.org/2/library/functions.html#super).

Ok, in this case there are wrong usage in more unittest (such as filereader_unittest.py)
Comment 12 László Langó 2013-11-22 05:40:43 PST
Created attachment 217673 [details]
Patch
Comment 13 Peter Gal 2013-11-22 06:05:50 PST
Comment on attachment 217673 [details]
Patch

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

> Tools/Scripts/webkitpy/style/error_handlers_unittest.py:39
> +        LoggingTestCase.setUp(self)

It would be better to use super(..) call here.
Comment 14 László Langó 2013-11-22 06:27:45 PST
Created attachment 217675 [details]
Patch
Comment 15 Peter Gal 2013-11-28 08:32:54 PST
(In reply to comment #14)
> Created an attachment (id=217675) [details]
> Patch

LGTM
Comment 16 WebKit Commit Bot 2013-12-02 15:49:47 PST
Comment on attachment 217675 [details]
Patch

Clearing flags on attachment: 217675

Committed r159977: <http://trac.webkit.org/changeset/159977>
Comment 17 WebKit Commit Bot 2013-12-02 15:49:49 PST
All reviewed patches have been landed.  Closing bug.