Bug 37850 – check-webkit-style: Add __eq__ and __ne__ methods to DefaultStyleErrorHandler

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This will facilitate unit testing for bug 37066.

[Chris Jerdonek]

Created attachment 53796 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 53798 [details]
Proposed patch 2

Tweaked two docstrings (added missing "the").

[Shinichiro Hamaji]

Comment on attachment 53796 [details]
Proposed patch

Looks good.

> +    def test_eq__true_return_value(self):

Maybe too many underscores after "eq"?

> +    def test_eq__false_return_value(self):

Maybe too many underscores after "eq"?

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 53796 [details])
> Looks good.
> 
> > +    def test_eq__true_return_value(self):
> 
> Maybe too many underscores after "eq"?
> 
> > +    def test_eq__false_return_value(self):
> 
> Maybe too many underscores after "eq"?

This is a style I learned from working on the Python source code and that I think is nice.

The style is to give unit tests names of the form--

test_methodbeingtested__usecasedescription

The double underscore is helpful because "methodbeingtested" itself often has underscores.

You can see an example of this style in the unit tests for the unittest module itself:

http://svn.python.org/view/python/trunk/Lib/unittest/test/test_loader.py?revision=79464&view=markup

Do you think this style is okay?  Note that this is a somewhat unusual case to start using the style with since __eq__ itself begins and ends with two underscores.  But it is probably best to leave those out of "methodbeingtested" in this case, or else we would have four consecutive underscores in the name. :)

[Chris Jerdonek]

If you'd like, for consistency I could adjust the names of the other test methods in the class.

They would like something like--

test_call__non_reportable_error

instead of--

test_non_reportable_error

By the way, thanks for reviewing so quickly!

[Shinichiro Hamaji]

Comment on attachment 53798 [details]
Proposed patch 2

I see. Thanks for your explanation. I have no preference for this. Please name methods as you like. I guess it's worth writing this rule in our python guideline?

[Chris Jerdonek]

(In reply to comment #6)
> (From update of attachment 53798 [details])
> I see. Thanks for your explanation. I have no preference for this. Please name
> methods as you like. I guess it's worth writing this rule in our python
> guideline?

Good suggestion.  I added a note about this on the wiki page:

https://trac.webkit.org/wiki/PythonGuidelines#Style

[WebKit Commit Bot]

Comment on attachment 53798 [details]
Proposed patch 2

Clearing flags on attachment: 53798

Committed r57905: <http://trac.webkit.org/changeset/57905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57905 might have broken Windows Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/57904
http://trac.webkit.org/changeset/57905
http://trac.webkit.org/changeset/57906
http://trac.webkit.org/changeset/57907
http://trac.webkit.org/changeset/57908