test(spanner): replace fixed hashCode value assertions with contract-compliance tests in OptionsTest#12901
test(spanner): replace fixed hashCode value assertions with contract-compliance tests in OptionsTest#12901anishesg wants to merge 1 commit intogoogleapis:mainfrom
Conversation
…compliance tests in OptionsTest `OptionsTest` contained four assertions that verified `Options#hashCode()` returns specific fixed integer values (31, 108027089, -2118248262). The Java `hashCode()` contract does not guarantee cross-JVM or cross-version stability of hash values, so testing against hardcoded numbers is brittle and misleads readers into thinking fixed values are part of the API contract. Signed-off-by: anish k <ak8686@princeton.edu>
There was a problem hiding this comment.
Code Review
This pull request updates OptionsTest.java to replace hardcoded hash code values with dynamic assertions that verify the hashCode contract between equal objects. The review feedback suggests removing redundant tautological assertions where an object's hash code is compared to itself and recommends using more idiomatic AssertJ assertions for object equality to provide better failure messages.
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | ||
| Options options2 = Options.fromReadOptions(); | ||
| assertThat(options.equals(options2)).isTrue(); | ||
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | ||
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); |
There was a problem hiding this comment.
The assertion assertThat(options.hashCode()).isEqualTo(options.hashCode()) is a tautology and provides little value in a unit test for a value-like object. Additionally, using assertThat(options).isEqualTo(options2) is more idiomatic and provides better failure messages than assertThat(options.equals(options2)).isTrue().
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | |
| Options options2 = Options.fromReadOptions(); | |
| assertThat(options.equals(options2)).isTrue(); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | |
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); | |
| // Verify hashCode contract: equal objects must have equal hashCodes. | |
| Options options2 = Options.fromReadOptions(); | |
| assertThat(options).isEqualTo(options2); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); |
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | ||
| Options options2 = | ||
| Options.fromListOptions( | ||
| Options.pageSize(pageSize), Options.pageToken(pageToken), Options.filter(filter)); | ||
| assertThat(options.equals(options2)).isTrue(); | ||
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | ||
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); |
There was a problem hiding this comment.
The assertion assertThat(options.hashCode()).isEqualTo(options.hashCode()) is a tautology and provides little value in a unit test for a value-like object. Additionally, using assertThat(options).isEqualTo(options2) is more idiomatic and provides better failure messages than assertThat(options.equals(options2)).isTrue().
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | |
| Options options2 = | |
| Options.fromListOptions( | |
| Options.pageSize(pageSize), Options.pageToken(pageToken), Options.filter(filter)); | |
| assertThat(options.equals(options2)).isTrue(); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | |
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); | |
| // Verify hashCode contract: equal objects must have equal hashCodes. | |
| Options options2 = | |
| Options.fromListOptions( | |
| Options.pageSize(pageSize), Options.pageToken(pageToken), Options.filter(filter)); | |
| assertThat(options).isEqualTo(options2); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); |
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | ||
| Options options2 = Options.fromUpdateOptions(Options.tag(tag)); | ||
| assertThat(options.equals(options2)).isTrue(); | ||
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | ||
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); |
There was a problem hiding this comment.
The assertion assertThat(options.hashCode()).isEqualTo(options.hashCode()) is a tautology and provides little value in a unit test for a value-like object. Additionally, using assertThat(options).isEqualTo(options2) is more idiomatic and provides better failure messages than assertThat(options.equals(options2)).isTrue().
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | |
| Options options2 = Options.fromUpdateOptions(Options.tag(tag)); | |
| assertThat(options.equals(options2)).isTrue(); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | |
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); | |
| // Verify hashCode contract: equal objects must have equal hashCodes. | |
| Options options2 = Options.fromUpdateOptions(Options.tag(tag)); | |
| assertThat(options).isEqualTo(options2); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); |
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | ||
| Options options2 = Options.fromTransactionOptions(Options.tag(tag)); | ||
| assertThat(options.equals(options2)).isTrue(); | ||
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | ||
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); |
There was a problem hiding this comment.
The assertion assertThat(options.hashCode()).isEqualTo(options.hashCode()) is a tautology and provides little value in a unit test for a value-like object. Additionally, using assertThat(options).isEqualTo(options2) is more idiomatic and provides better failure messages than assertThat(options.equals(options2)).isTrue().
| // Verify hashCode contract: equal objects must have equal hashCodes, and hashCode is stable. | |
| Options options2 = Options.fromTransactionOptions(Options.tag(tag)); | |
| assertThat(options.equals(options2)).isTrue(); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); | |
| assertThat(options.hashCode()).isEqualTo(options.hashCode()); | |
| // Verify hashCode contract: equal objects must have equal hashCodes. | |
| Options options2 = Options.fromTransactionOptions(Options.tag(tag)); | |
| assertThat(options).isEqualTo(options2); | |
| assertThat(options.hashCode()).isEqualTo(options2.hashCode()); |
OptionsTestcontained four assertions that verifiedOptions#hashCode()returns specific fixed integer values (31, 108027089, -2118248262). The JavahashCode()contract does not guarantee cross-JVM or cross-version stability of hash values, so testing against hardcoded numbers is brittle and misleads readers into thinking fixed values are part of the API contract.This change replaces each fixed-value assertion in
allOptionsAbsent,listOptionsTest,updateOptionsTest, andtransactionOptionsTestwith contract-compliance assertions: two equalOptionsinstances produce equal hash codes, and callinghashCode()twice on the same instance returns the same value (stability within a session).Fixes #12268