New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests + Fixes for Equals/Hashcode/toString of Conditions #276

Merged
merged 21 commits into from Jun 11, 2013

Conversation

Projects
None yet
3 participants
@avandeursen
Member

avandeursen commented Jun 10, 2013

Wrote (parameterized) test suite for equals/hashCode/toString in the Condition class hierarchy, in new test class ConditionTest.

Fixed equals/hashCode/toString of the 11 classes providing Condition implementations by

  • Removing super calls to Object.{equals, hashCode, toString}
  • Adding the class-object itself to the computation of the hashCode
  • Replacing arguments without equals method to their .toString counter parts in equals calls

The equals/hashCode methods in the Condition hierarchy now behave nicely.

As a bonus I removed some unnecessary AtomicInteger from CountCondition (which is quite tricky anyway as the check method has a side effect). Also, the counter itself is not included in the equality check atm.

Some refactoring of the Condition hierarchy is in place though -- see also issue #275.

avandeursen added some commits May 27, 2013

Wrote test cases for equality, and fixed equality in RegexCond
Wrote test cases to ensure consistency between hashCode, toString, 
and equals. Had to fix RegexCond accordingly.

Removed the super.equals, super.hashcode, and super.toString calls,
since these revert back to Object, which in turn gives pointer
equivalence. (Such super calls are only useful if the superclass 
defines structurally equivalence as well).

Since the Pattern class has no structural equivalence, I replaced
the Pattern equality check used in equals by an equality check on
the String representation.

Todo: 

* fix other condition Condition classes having same problem;
* turn test cases into parameterized test covering all Conditions.
Testcase for (Not)XPathCondition + fix of equals/hashcode.
Also added explanatory string in Parameterized test case.
Added 9 failing test cases if args are same but constructor differs
Hashcode should not only look at the fields, but also at itself.
Fixed hashcode so that it looks at constructor and arguments.
Hashcode does this by taking 
this.getClass().getName() into account as well.
@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Jun 9, 2013

Member

Why use getname() in stead of just getClass and let their hashCode() work it out? It's best practice to call hashCode() on other objects when constructing your own hashCode. That way, the responsibility of generating a useful hashcode is left up to the class you are calling.

Why use getname() in stead of just getClass and let their hashCode() work it out? It's best practice to call hashCode() on other objects when constructing your own hashCode. That way, the responsibility of generating a useful hashcode is left up to the class you are calling.

This comment has been minimized.

Show comment
Hide comment
@avandeursen

avandeursen Jun 9, 2013

Member

Good point. Will remove the getName-calls.

Member

avandeursen replied Jun 9, 2013

Good point. Will remove the getName-calls.

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Jun 9, 2013

Member

This is equivalent to return Objects.hashCode(this.getClass().getName(), conditions); right?

This is equivalent to return Objects.hashCode(this.getClass().getName(), conditions); right?

This comment has been minimized.

Show comment
Hide comment
@avandeursen

avandeursen Jun 9, 2013

Member

No.

Objects.hashCode takes one vararg.
The red old code was OK, in that it 'expanded' the array to individual arguments.

The code you suggest is indeed what I'd want to write (and initially wrote).
However, it does not expand the conditions array to the individual conditions, and hence does a hashcode computation on the array pointer, instead of on the values of the array elements.

Member

avandeursen replied Jun 9, 2013

No.

Objects.hashCode takes one vararg.
The red old code was OK, in that it 'expanded' the array to individual arguments.

The code you suggest is indeed what I'd want to write (and initially wrote).
However, it does not expand the conditions array to the individual conditions, and hence does a hashcode computation on the array pointer, instead of on the values of the array elements.

alexnederlof added a commit that referenced this pull request Jun 11, 2013

Merge pull request #276 from crawljax/testing-conditions
Tests + Fixes for Equals/Hashcode/toString of Conditions

@alexnederlof alexnederlof merged commit 572753d into master Jun 11, 2013

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Jun 11, 2013

Member

Very nice!!

Member

alexnederlof commented Jun 11, 2013

Very nice!!

@alexnederlof alexnederlof deleted the testing-conditions branch Jun 11, 2013

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Jun 11, 2013

Member

@avandeursen I suspect you didn't use the crawljax eclipse formatter. Could you double check this next time you contribute? Makes the diffs more understandable.

Member

alexnederlof commented Jun 11, 2013

@avandeursen I suspect you didn't use the crawljax eclipse formatter. Could you double check this next time you contribute? Makes the diffs more understandable.

@avandeursen

This comment has been minimized.

Show comment
Hide comment
Member

avandeursen commented Jun 11, 2013

👍

@amesbah

This comment has been minimized.

Show comment
Hide comment
@amesbah

amesbah Jun 11, 2013

Member

Great job!

Member

amesbah commented Jun 11, 2013

Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment