Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR elastic#138668

Type: Corrupted (contains bugs)

Original PR Title: Test utility for BytesReference equality
Original PR Description: 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.
Original PR URL: elastic#138668


PR Type

Tests, Enhancement


Description

  • Introduces equalBytes() matcher for improved BytesReference equality testing

  • Provides detailed hex output on assertion failures for better debugging

  • Replaces equalTo() with equalBytes() across 60+ test files

  • Adds comprehensive test coverage for the new matcher utility


Diagram Walkthrough

flowchart LR
  A["BytesReferenceTestUtils<br/>new matcher"] -->|used in| B["60+ test files"]
  A -->|replaces| C["equalTo for<br/>BytesReference"]
  A -->|provides| D["Hex output<br/>on mismatch"]
  B -->|improves| E["Test debugging<br/>experience"]
Loading

File Walkthrough

Relevant files
Enhancement
1 files
BytesReferenceTestUtils.java
New matcher utility for BytesReference equality testing   
+77/-0   
Tests
60 files
BytesReferenceTestUtilsTests.java
Comprehensive tests for equalBytes matcher functionality 
+49/-0   
PointInTimeIT.java
Replace equalTo with equalBytes for BytesReference assertions
+15/-14 
ApiKeyServiceTests.java
Update BytesReference assertions to use equalBytes matcher
+25/-18 
FsBlobContainerTests.java
Replace assertEquals with assertThat using equalBytes matcher
+16/-14 
ClientTransformIndexerTests.java
Update PIT ID assertions to use equalBytes matcher             
+11/-10 
RestRequestTests.java
Replace equalTo with equalBytes for content assertions     
+12/-11 
AbstractBytesReferenceTestCase.java
Update BytesReference equality tests to use equalBytes     
+8/-7     
DefaultRestChannelTests.java
Replace equalTo with equalBytes for response content         
+8/-7     
AbstractThirdPartyRepositoryTestCase.java
Update blob content assertions to use equalBytes matcher 
+5/-4     
BulkRequestTests.java
Replace equalTo with equalBytes for source assertions       
+4/-3     
S3RepositoryThirdPartyTests.java
Update register read assertions to use equalBytes               
+3/-2     
BytesStreamsTests.java
Replace equalTo with equalBytes for stream assertions       
+4/-3     
GoogleCloudStorageBlobContainerRetriesTests.java
Update blob content assertions to use equalBytes matcher 
+3/-2     
GoogleCloudStorageBlobContainerStatsTests.java
Replace assertEquals with assertThat using equalBytes       
+3/-2     
RecyclerBytesStreamOutputTests.java
Update stream output assertions to use equalBytes matcher
+4/-3     
RetrySearchIntegTests.java
Replace equalTo with equalBytes for PIT assertions             
+5/-4     
ChunkedRestResponseBodyPartTests.java
Update chunked response assertions to use equalBytes         
+4/-2     
ClientScrollableHitSourceTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
RetryingInputStreamTests.java
Update resource bytes assertions to use equalBytes matcher
+3/-2     
CompositeBytesReferenceTests.java
Replace assertEquals with assertThat using equalBytes       
+4/-3     
GetActionIT.java
Update source bytes assertions to use equalBytes matcher 
+4/-3     
ShardBulkInferenceActionFilterTests.java
Replace equalTo with equalBytes for source assertions       
+4/-3     
RoundTripTests.java
Update remote query assertions to use equalBytes matcher 
+2/-1     
PagedBytesReferenceTests.java
Replace assertEquals with assertThat using equalBytes       
+3/-1     
BlobContainerUtilsTests.java
Update register content assertions to use equalBytes         
+2/-1     
ChunkedLoggingStreamTests.java
Replace assertEquals with assertThat using equalBytes       
+7/-8     
RestGetSourceActionTests.java
Update response content assertions to use equalBytes         
+2/-1     
GetAutoscalingCapacityActionResponseTests.java
Replace Matchers.equalTo with equalBytes for response       
+3/-2     
Netty4HttpPipeliningHandlerTests.java
Update content assertions to use equalBytes matcher           
+2/-1     
SearchFieldsIT.java
Replace equalTo with equalBytes for binary field assertions
+2/-1     
EngineTestCase.java
Update translog source assertions to use equalBytes           
+2/-1     
AuthenticationTests.java
Update role descriptors assertions to use equalBytes         
+2/-1     
HttpTracerTests.java
Replace assertEquals with assertThat using equalBytes       
+5/-3     
ShardSearchRequestTests.java
Update cache key assertions to use equalBytes matcher       
+2/-1     
GoogleCloudStorageBlobStoreRepositoryTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
GetFieldMappingsResponseTests.java
Update field mapping source assertions to use equalBytes 
+3/-1     
ChunkedTrainedModelPersisterIT.java
Replace equalTo with equalBytes for model definition         
+3/-2     
TermVectorsUnitTests.java
Update doc assertions to use equalBytes matcher                   
+2/-1     
TranslogTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
InnerHitBuilderTests.java
Update serialization assertions to use equalBytes matcher
+2/-1     
TranslogHeaderWriterTests.java
Replace equalTo with equalBytes for serialization               
+2/-1     
PITAwareQueryClientTests.java
Update PIT ID assertions to use equalBytes matcher             
+3/-2     
DeflateCompressedXContentTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
BytesStreamOutputTests.java
Update stream bytes assertions to use equalBytes                 
+3/-1     
CancellationTests.java
Update PIT ID assertions to use equalBytes matcher             
+3/-2     
RestContentAggregatorTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
IndexRequestTests.java
Update source assertions to use equalBytes matcher             
+2/-1     
MultipartUploadTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
RestMainActionTests.java
Update response content assertions to use equalBytes         
+2/-1     
PutPipelineRequestTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
LongBoundsTests.java
Update transport round trip assertions to use equalBytes 
+2/-1     
XContentDataHelperTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
DoubleBoundsTests.java
Update transport round trip assertions to use equalBytes 
+2/-1     
SubjectTests.java
Update role descriptors assertions to use equalBytes         
+2/-1     
OutboundHandlerTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
NativePrivilegeStoreTests.java
Update source assertions to use equalBytes matcher             
+2/-1     
DeflateCompressTests.java
Replace assertEquals with assertThat using equalBytes       
+2/-1     
RepositoryAnalysisSuccessIT.java
Update register value assertions to use equalBytes             
+2/-1     
DatafeedJobTests.java
Update source assertions to use equalBytes matcher             
+2/-1     
MonitoringBulkRequestTests.java
Replace equalTo with equalBytes for source assertions       
+2/-1     
Additional files
3 files
SamlAuthenticationStateTests.java +2/-1     
MonitoringBulkDocTests.java +2/-1     
BytesReferenceMonitoringDocTests.java +2/-1     

DaveCTurner and others added 2 commits November 26, 2025 13:51
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.
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Test Scope Only: The PR primarily updates test assertions and introduces a test utility without adding or
modifying any runtime audit logging of critical actions, which appears out of scope for
this compliance item.

Referred Code
refresh("test");
BytesReference pitId = openPointInTime(new String[] { "test" }, TimeValue.timeValueMinutes(2)).getPointInTimeId();
assertResponse(prepareSearch().setPointInTime(new PointInTimeBuilder(pitId)), resp1 -> {
    assertThat(resp1.pointInTimeId(), equalBytes(pitId));
    assertHitCount(resp1, numDocs);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Matcher Robustness: The new matcher’s iterator loop and hex rendering assume normal iteration and may risk
IndexOutOfBounds due to using <= length and lacks explicit handling for unusual
BytesReference implementations, which may require review though it is test-only.

Referred Code
while ((bytesRef = iterator.next()) != null) {
    for (int i = 0; i <= bytesRef.length; i++) {
        if (first) {
            first = false;
        } else {
            stringBuilder.append(' ');
        }
        stringBuilder.append(Strings.format("%02x", bytesRef.bytes[bytesRef.offset + i]));
    }

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix off-by-one error in loop

Fix an off-by-one error in the appendBytesReferenceDescription method by
changing the loop condition from i <= bytesRef.length to i < bytesRef.length to
prevent an ArrayIndexOutOfBoundsException.

test/framework/src/main/java/org/elasticsearch/common/bytes/BytesReferenceTestUtils.java [44-64]

 private void appendBytesReferenceDescription(BytesReference bytesReference, Description description) {
     final var stringBuilder = new StringBuilder("BytesReference[");
     final var iterator = bytesReference.iterator();
     BytesRef bytesRef;
     boolean first = true;
     try {
         while ((bytesRef = iterator.next()) != null) {
-            for (int i = 0; i <= bytesRef.length; i++) {
+            for (int i = 0; i < bytesRef.length; i++) {
                 if (first) {
                     first = false;
                 } else {
                     stringBuilder.append(' ');
                 }
                 stringBuilder.append(Strings.format("%02x", bytesRef.bytes[bytesRef.offset + i]));
             }
         }
         description.appendText(stringBuilder.append(']').toString());
     } catch (IOException e) {
         throw new AssertionError("no IO happens here", e);
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical off-by-one error in the loop condition i <= bytesRef.length which would cause an ArrayIndexOutOfBoundsException when describing a byte reference, thus breaking the new test utility.

High
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants