Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

Today if you assert two BytesReference objects are equal, and they
aren't, then the test output is unhelpful because BytesReference
instances do not include their contents in their default string
representations. This commit introduces a new matcher specifically for
testing BytesReference instances for equality which generates more
useful output.

Today if you assert two `BytesReference` objects are equal, and they
aren't, then the test output is unhelpful because `BytesReference`
instances do not include their contents in their default string
representations. This commit introduces a new matcher specifically for
testing `BytesReference` instances for equality which generates more
useful output.
@DaveCTurner DaveCTurner requested a review from a team as a code owner November 26, 2025 13:52
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. v9.3.0 labels Nov 26, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 26, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

@DaveCTurner
Copy link
Contributor Author

Review notes: apart from BytesReferenceTestUtils and its test suite in BytesReferenceTestUtilsTests this was all done mechanically via structural search-and-replace (plus a bit of manual cleanup).

Copy link
Contributor

@joshua-adams-1 joshua-adams-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. General question: how did you find every instance of assertEquals that had two ByteReference's passed in, or an existing assertThat that had equalsTo(ByteReference) ? When I look on Intellij, I see a function definition such as private static boolean isEquals(Object expected, Object actual) { which isn't helpful in this case.

import java.util.Objects;

public enum BytesReferenceTestUtils {
;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit] Extra semicolon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's required - omitting it is a compile error 😁

Copy link
Contributor

@mark-vieira mark-vieira Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using enums for static utilities a pattern we're using now? Curious about this choice vs an abstract class or final class with a private constructor to accomplish the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do both and this is my preference purely because it involves less boilerplate than declaring a private constructor.

@DaveCTurner
Copy link
Contributor Author

how did you find every instance of assertEquals that had two ByteReference's passed in

Structural search & replace (under Edit -> Find -> Replace Structurally...)

image

@DaveCTurner DaveCTurner merged commit 535ba6b into elastic:main Nov 26, 2025
34 checks passed
@DaveCTurner DaveCTurner deleted the 2025/11/26/equalBytes branch November 26, 2025 16:03
@mark-vieira
Copy link
Contributor

Review notes: apart from BytesReferenceTestUtils and its test suite in BytesReferenceTestUtilsTests this was all done mechanically via structural search-and-replace (plus a bit of manual cleanup).

SSR is incredibly powerful. I used it extensively in our work to remove old usages of unsupported transport versions.

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

Labels

:Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. Team:Distributed Coordination Meta label for Distributed Coordination team >test Issues or PRs that are addressing/adding tests v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants