Bug 52002 – commit-queue mentions "Text diff mismatch" 4 times instead of once per failure

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

commit-queue mentions "Text diff mismatch" 4 times instead of once per failure

[Eric Seidel (no email)]

Created attachment 78143 [details]
Patch

[Mihai Parparita]

Comment on attachment 78143 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:88
> +    # FIXME: We don't actually use TestFailure objects as instances,

You mean that we don't use TestFailure directly, but instead use subclaseses like FailureCrash? Perhaps the comment could be more explicit.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:97
> +        # An attempt to make sure that __class__ and self don't hash to the same value.

Though it may not matter in practice, it seems like the implementation of this should just be hash(self.__class__.__name__), since you never compare self.__class__ directly in __eq__. (roughly, you should only hash things that you compare in __eq__, otherwise they'll end up in different buckets even though they would pass the equality test).

[Eric Seidel (no email)]

Comment on attachment 78143 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:88

> 
> You mean that we don't use TestFailure directly, but instead use subclaseses like FailureCrash? Perhaps the comment could be more explicit.

We basically always pass around FooFailure() instead of FooFailure (the class) directly, yet we *never* actually store any state on the instances.

I want to re-write this module so that it works more like tool/steps does.  Where you use it by importing the module, and then reference these as test_failures.Crash and don't ever bother instantiting one.  Basically like a big enum.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:97

> 
> Though it may not matter in practice, it seems like the implementation of this should just be hash(self.__class__.__name__), since you never compare self.__class__ directly in __eq__. (roughly, you should only hash things that you compare in __eq__, otherwise they'll end up in different buckets even though they would pass the equality test).

I was trying to avoid having hash(FooFailure) == hash("FooFailure")

isn't __class__ as singleton?  So it should always have the same hash?  I could alternatively just do hash(sef.__class__.__name__) + 5, to avoid it being the same hash as "FooFailure"

[Eric Seidel (no email)]

Created attachment 78152 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 78152 [details]
Patch

Clearing flags on attachment: 78152

Committed r75191: <http://trac.webkit.org/changeset/75191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.