Skip to content

[test] Add fuzz test for SpanRef compatibility#188

Open
chethanm99 wants to merge 14 commits intojaegertracing:mainfrom
chethanm99:fuzz-spanref-compatibility
Open

[test] Add fuzz test for SpanRef compatibility#188
chethanm99 wants to merge 14 commits intojaegertracing:mainfrom
chethanm99:fuzz-spanref-compatibility

Conversation

@chethanm99
Copy link

@chethanm99 chethanm99 commented Mar 3, 2026

Which problem is this PR solving?

  • Adds fuzz testing to check SpanRef compatibility.

Description of the changes

  • Adds a fuzzing test for model.SpanRef.
  • Checks the compatibility between custom model types with standard protobuf output.

How was this change tested?

  • Ran go test ./... and executed fuzzing test locally using: go test -fuzz=FuzzSpanRef -fuzztime=20s

Checklist

AI Usage in this PR (choose one)

See AI Usage Policy.

  • None: No AI tools were used in creating this PR
  • Light: AI provided minor assistance (formatting, simple suggestions)
  • Moderate: AI helped with code generation or debugging specific parts
  • Heavy: AI generated most or all of the code changes

Copilot AI review requested due to automatic review settings March 3, 2026 06:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a Go fuzz test to validate that model.SpanRef (with custom TraceID/SpanID types) stays wire-compatible with a “plain” protobuf-generated SpanRef message, and removes the old TODO note suggesting fuzzing.

Changes:

  • Add FuzzSpanRef to compare protobuf binary encoding between model.SpanRef and prototest.SpanRef.
  • Add protobuf and JSON roundtrip checks for model.SpanRef inside the fuzz harness.
  • Remove the obsolete TODO comment from ids_proto_test.go.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
model/v1/spanref_fuzz_test.go Introduces fuzz coverage for SpanRef serialization compatibility and roundtrips.
model/v1/ids_proto_test.go Removes a TODO comment now addressed by the new fuzz test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +35 to +36
_, _ = traceID.MarshalTo(traceBytes)
_, _ = spanID.MarshalTo(spanBytes)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The return values from traceID.MarshalTo / spanID.MarshalTo are currently ignored. These methods can return an error (e.g., if the destination buffer is too short), and ignoring it would make failures harder to diagnose in fuzz runs. Please check the error (and optionally assert the number of bytes written matches the buffer length).

Suggested change
_, _ = traceID.MarshalTo(traceBytes)
_, _ = spanID.MarshalTo(spanBytes)
if n, err := traceID.MarshalTo(traceBytes); err != nil {
t.Fatalf("traceID marshal failed: %v", err)
} else if n != len(traceBytes) {
t.Fatalf("traceID marshal wrote %d bytes, expected %d", n, len(traceBytes))
}
if n, err := spanID.MarshalTo(spanBytes); err != nil {
t.Fatalf("spanID marshal failed: %v", err)
} else if n != len(spanBytes) {
t.Fatalf("spanID marshal wrote %d bytes, expected %d", n, len(spanBytes))
}

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +41
f.Fuzz(func(t *testing.T, high, low, span uint64) {
//Construct SpanRef using custom model types.
ref1 := model.SpanRef{
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
}

//Convert traceID and spanID into raw byte format before
//constructing SpanRefs.
traceID := model.NewTraceID(high, low)
spanID := model.NewSpanID(span)

traceBytes := make([]byte, traceID.Size())
spanBytes := make([]byte, spanID.Size())

_, _ = traceID.MarshalTo(traceBytes)
_, _ = spanID.MarshalTo(spanBytes)

ref2 := prototest.SpanRef{
TraceId: traceBytes,
SpanId: spanBytes,
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This fuzz test currently only exercises the default ref_type value (both ref1 and ref2 leave it at 0). Since the goal is SpanRef compatibility, consider fuzzing RefType as well (e.g., add an extra input that maps to the known enum values and set it on both messages) so non-default encodings/JSON are covered.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +3
package model_test

import (
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This new test file is missing the standard Jaeger copyright/SPDX header that other files in model/v1 include. Please add the same header block at the top to match repository licensing conventions.

Copilot uses AI. Check for mistakes.
Comment on lines +15 to +22
//Add seed inputs to cover normal, zero and max boundary values.
f.Add(uint64(2), uint64(3), uint64(11))
f.Add(uint64(0), uint64(0), uint64(0))
f.Add(^uint64(0), ^uint64(0), ^uint64(0))

f.Fuzz(func(t *testing.T, high, low, span uint64) {
//Construct SpanRef using custom model types.
ref1 := model.SpanRef{
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Several comments don’t follow the usual Go style in this repo (a space after //, and sentence-style capitalization). Please update them for consistency and readability.

Copilot uses AI. Check for mistakes.
@chethanm99 chethanm99 changed the title Add fuzz test for SpanRef compatibility [test] Add fuzz test for SpanRef compatibility Mar 3, 2026
@chethanm99 chethanm99 requested a review from Copilot March 3, 2026 07:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

f.Add(^uint64(0), ^uint64(0), ^uint64(0), uint8(0))

f.Fuzz(func(t *testing.T, high, low, span uint64, refType uint8) {
rt := model.SpanRefType(refType % 2)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Hard-coding refType % 2 assumes the SpanRefType enum will always have exactly two values. Since this is a compatibility fuzz test, it would be more future-proof to select among the known enum constants (e.g., via a small slice or switch) so the test fails loudly / expands coverage if new enum values are added.

Suggested change
rt := model.SpanRefType(refType % 2)
refTypes := []model.SpanRefType{
model.SpanRefType_CHILD_OF,
model.SpanRefType_FOLLOWS_FROM,
}
if len(model.SpanRefType_name) != len(refTypes) {
t.Fatalf("SpanRefType enum has changed; update FuzzSpanRef refTypes slice")
}
rt := refTypes[int(refType)%len(refTypes)]

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +37
ref1 := model.SpanRef{
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
RefType: rt,
}

// Convert traceID and spanID into raw byte format before
// constructing SpanRefs.
traceID := model.NewTraceID(high, low)
spanID := model.NewSpanID(span)

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

TraceID/SpanID are constructed twice from the same inputs (once in ref1, then again for marshaling to bytes). Reuse the already-created values (or build ref1 from traceID/spanID) to keep the fuzz body simpler and avoid redundant work.

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +102
out := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out, &ref1); err != nil {
t.Fatalf("json marshal failed: %v", err)
}

var ref1j model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out.Bytes()), &ref1j); err != nil {
t.Fatalf("json unmarshal failed: %v", err)
}

if !proto.Equal(&ref1, &ref1j) {
t.Fatalf("json roundtrip mismatch")
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The JSON section only verifies that ref1 JSON marshals and round-trips back into ref1. If the goal is compatibility with the “standard” (non-gogo) proto representation (like ids_proto_test.go), also marshal ref2 to JSON and compare the JSON outputs (and/or unmarshal ref2 JSON into model.SpanRef) so fuzzing can catch divergences in JSON encoding, not just protobuf binary encoding.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +100 to +112
out2 := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out2, &ref1); err != nil {
t.Fatalf("json marshal ref1 failed: %v", err)
}

var j1, j2 model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out1.Bytes()), &j1); err != nil {
t.Fatalf("json unmarshal j1 failed: %v", err)
}

if err := jsonpb.Unmarshal(bytes.NewReader(out1.Bytes()), &j2); err != nil {
t.Fatalf("json unmarshal j2 failed: %v", err)
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The JSON compatibility section currently never exercises the prototest.SpanRef: out2 is marshaled from ref1 (not ref2), and j2 is unmarshaled from out1 (not out2). As written, j1 and j2 will always be identical and this won't catch JSON incompatibilities. Marshal ref2 into out2, unmarshal out2 for j2, and/or directly compare the two JSON payloads produced from ref1 vs ref2.

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +78
t.Fatalf("marshal ref1 failed")
}

d2, err := proto.Marshal(&ref2)
if err != nil {
t.Fatalf("marshal ref2 failed")
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

On marshal failures the t.Fatalf calls drop the underlying error (e.g., t.Fatalf("marshal ref1 failed")). Including the err value (and ideally which marshal path failed) makes fuzz findings actionable and avoids losing the root cause in CI logs.

Suggested change
t.Fatalf("marshal ref1 failed")
}
d2, err := proto.Marshal(&ref2)
if err != nil {
t.Fatalf("marshal ref2 failed")
t.Fatalf("marshal ref1 failed: %v", err)
}
d2, err := proto.Marshal(&ref2)
if err != nil {
t.Fatalf("marshal ref2 failed: %v", err)

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +22
var enumValues []model.SpanRefType
for v := range model.SpanRefType_name {
enumValues = append(enumValues, model.SpanRefType(v))
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

enumValues is built by ranging over model.SpanRefType_name, which is a map, so the slice order is nondeterministic. That makes the seed values for refType unstable across runs (e.g., refType==0 may not consistently map to the same SpanRefType). Consider using an explicit, deterministic list of enum constants (or sort the keys) so seeds are meaningful and reproducible.

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +38
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

traceID/spanID are computed, but ref1 reconstructs them again (model.NewTraceID(high, low) / model.NewSpanID(span)). Reusing the already-created traceID/spanID in ref1 avoids duplication and ensures the bytes used for ref2 and the IDs in ref1 can’t diverge if construction ever changes.

Suggested change
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
TraceID: traceID,
SpanID: spanID,

Copilot uses AI. Check for mistakes.
@chethanm99 chethanm99 requested a review from Copilot March 3, 2026 10:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

RefType: rt,
}

//Convert TraceID and SpanID into raw bytes.
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Add a space after // to follow Go comment style and improve readability.

Suggested change
//Convert TraceID and SpanID into raw bytes.
// Convert TraceID and SpanID into raw bytes.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +43
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

traceID/spanID are computed but then ref1 reconstructs them from the same inputs. Reuse the existing traceID and spanID variables in ref1 to avoid duplication and keep the test’s data flow simpler.

Suggested change
TraceID: model.NewTraceID(high, low),
SpanID: model.NewSpanID(span),
TraceID: traceID,
SpanID: spanID,

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +100 to +106
out1 := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out1, &ref1); err != nil {
t.Fatalf("json marshal ref1 failed: %v", err)
}

out2 := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out2, &ref2); err != nil {
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This fuzz target allocates new buffers and new jsonpb.Marshaler instances on every iteration, which can significantly reduce fuzzing throughput. Consider reusing buffers (Reset) and reusing a single marshaler/unmarshaler instance within the fuzz function to keep allocations down.

Suggested change
out1 := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out1, &ref1); err != nil {
t.Fatalf("json marshal ref1 failed: %v", err)
}
out2 := new(bytes.Buffer)
if err := new(jsonpb.Marshaler).Marshal(out2, &ref2); err != nil {
marshaler := &jsonpb.Marshaler{}
var out1, out2 bytes.Buffer
if err := marshaler.Marshal(&out1, &ref1); err != nil {
t.Fatalf("json marshal ref1 failed: %v", err)
}
if err := marshaler.Marshal(&out2, &ref2); err != nil {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

if !bytes.Equal(d1, d2) {
t.Fatalf("profound encoding mismatch")
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The failure message has a typo: "profound encoding mismatch" reads like it should be referring to protobuf/proto encoding. Consider correcting the wording so failures are clearer when fuzzing finds a counterexample.

Suggested change
t.Fatalf("profound encoding mismatch")
t.Fatalf("protobuf encoding mismatch between model.SpanRef and prototest.SpanRef")

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


if !proto.Equal(&j1, &j2) {
t.Fatalf("json encoding mismatch between model and prototest")
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The JSON section only checks that both JSON encodings decode to the same model.SpanRef, but it doesn't assert that the decoded value matches the original ref1. Adding an equality check against ref1 (for j1 and/or j2) would ensure the fuzz test also catches cases where both encodings drop or change the same data.

Suggested change
}
}
if !proto.Equal(&ref1, &j1) {
t.Fatalf("json roundtrip mismatched original ref1")
}

Copilot uses AI. Check for mistakes.
Comment on lines +100 to +101
var marshaler = &jsonpb.Marshaler{}

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The jsonpb.Marshaler is allocated on every fuzz iteration. Consider moving the marshaler creation outside the f.Fuzz closure (or using a shared instance) to reduce per-iteration allocations during fuzzing.

Copilot uses AI. Check for mistakes.
}

var ref1u model.SpanRef
if err := proto.Unmarshal(d1, &ref1u); err != nil {
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This unmarshal uses d1 (marshaled from model.SpanRef). Since the goal is compatibility with the "standard" SpanRef encoding, unmarshalling d2 (marshaled from prototest.SpanRef) would better validate that the model type can consume the reference encoding.

Suggested change
if err := proto.Unmarshal(d1, &ref1u); err != nil {
if err := proto.Unmarshal(d2, &ref1u); err != nil {

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: Chethan  <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +49 to +51
// Convert TraceID and SpanID into raw bytes.
traceBytes := make([]byte, traceID.Size())
spanBytes := make([]byte, spanID.Size())
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This fuzz loop allocates new traceBytes / spanBytes slices on every iteration. Since these sizes are fixed (TraceID is always 16 bytes; SpanID is always 8 bytes), consider using stack arrays (e.g., [16]byte / [8]byte) or reusing buffers to improve fuzzing throughput.

Suggested change
// Convert TraceID and SpanID into raw bytes.
traceBytes := make([]byte, traceID.Size())
spanBytes := make([]byte, spanID.Size())
// Convert TraceID and SpanID into raw bytes using stack-allocated buffers
var traceBytesArr [16]byte
var spanBytesArr [8]byte
traceBytes := traceBytesArr[:traceID.Size()]
spanBytes := spanBytesArr[:spanID.Size()]

Copilot uses AI. Check for mistakes.
return enumValues[i] < enumValues[j]
})

var marshaler = &jsonpb.Marshaler{}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

marshaler is assigned with var marshaler = ... but never reassigned. Using short declaration (marshaler := ...) keeps the test consistent with typical Go style in this package and reduces verbosity.

Suggested change
var marshaler = &jsonpb.Marshaler{}
marshaler := &jsonpb.Marshaler{}

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +105 to +125
var out1, out2 bytes.Buffer
if err := marshaler.Marshal(&out1, &ref1); err != nil {
t.Fatalf("json marshal ref1 failed: %v", err)
}

if err := marshaler.Marshal(&out2, &ref2); err != nil {
t.Fatalf("json marshal ref2 failed: %v", err)
}

var j1, j2 model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out1.Bytes()), &j1); err != nil {
t.Fatalf("json unmarshal j1 failed: %v", err)
}

if err := jsonpb.Unmarshal(bytes.NewReader(out2.Bytes()), &j2); err != nil {
t.Fatalf("json unmarshal j2 failed: %v", err)
}

if !proto.Equal(&j1, &j2) {
t.Fatalf("json encoding mismatch between model and prototest")
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The JSON compatibility check here compares the decoded messages (j1 vs j2) rather than the actual JSON encodings (out1 vs out2). This can miss real differences in the JSON output format (e.g., enum representation, field naming, default field emission) as long as both outputs happen to unmarshal into the same model.SpanRef. If the goal is to validate that model.SpanRef and prototest.SpanRef produce equivalent JSON encodings, consider comparing the marshaled JSON outputs directly (e.g., using a normalized/JSONEq comparison) and keep the unmarshal roundtrip assertions as a separate check.

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if err := jsonpb.Unmarshal(bytes.NewReader(out2.Bytes()), &j2); err != nil {
t.Fatalf("json unmarshal j2 failed: %v", err)
}

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

j1 and j2 are unmarshaled but never used, which will fail go test due to unused variables. Either remove these unmarshals or use them for an assertion (e.g., compare j1 and j2, and/or compare j1/j2 against ref1).

Suggested change
if !proto.Equal(&ref1, &j1) {
t.Fatalf("jsonpb roundtrip mismatch for ref1")
}
if !proto.Equal(&ref2, &j2) {
t.Fatalf("jsonpb roundtrip mismatch for ref2")
}

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +125 to +130
if !proto.Equal(&ref1, &j1) {
t.Fatalf("jsonpb roundtrip mismatch for ref1")
}
if !proto.Equal(&ref2, &j2) {
t.Fatalf("jsonpb roundtrip mismatch for ref2")
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

proto.Equal(&ref2, &j2) compares different message types (prototest.SpanRef vs model.SpanRef). proto.Equal typically returns false when the concrete types differ even if the serialized data is identical, which would make this fuzz test fail spuriously. Compare j2 against ref1 (same type) or unmarshal into a prototest.SpanRef and compare against ref2 instead.

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
@chethanm99 chethanm99 requested a review from Copilot March 3, 2026 12:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +154 to +161
var ref1j model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out1.Bytes()), &ref1j); err != nil {
t.Fatalf("json unmarshal failed: %v", err)
}

if !proto.Equal(&ref1, &ref1j) {
t.Fatalf("json roundtrip mismatch")
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

There’s duplicated work in this fuzz loop: out1 is unmarshaled into j1 (with equality asserted) and then later unmarshaled again into ref1j and re-asserted. Consider removing the second unmarshal/assert (lines 154–161), or reusing j1, to keep the fuzz body minimal and easier to maintain.

Suggested change
var ref1j model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out1.Bytes()), &ref1j); err != nil {
t.Fatalf("json unmarshal failed: %v", err)
}
if !proto.Equal(&ref1, &ref1j) {
t.Fatalf("json roundtrip mismatch")
}

Copilot uses AI. Check for mistakes.
Comment on lines +147 to +152
}

var cross2 model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out2.Bytes()), &cross2); err != nil {
t.Fatalf("prototest JSON not compatible with model: %v", err)
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

These “compatibility” checks only assert that cross-unmarshal succeeds, but they don’t assert that the decoded value is equivalent to the original (or that a re-marshal matches). To make the fuzz test catch real compatibility regressions, add equality checks (e.g., proto.Equal against ref2/ref1, or marshal-to-bytes comparisons) for cross1 and cross2.

Suggested change
}
var cross2 model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out2.Bytes()), &cross2); err != nil {
t.Fatalf("prototest JSON not compatible with model: %v", err)
}
}
if !proto.Equal(&j2, &cross1) {
t.Fatalf("model JSON/prototest cross-compatibility mismatch")
}
var cross2 model.SpanRef
if err := jsonpb.Unmarshal(bytes.NewReader(out2.Bytes()), &cross2); err != nil {
t.Fatalf("prototest JSON not compatible with model: %v", err)
}
if !proto.Equal(&ref1, &cross2) {
t.Fatalf("prototest JSON/model cross-compatibility mismatch")
}

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

t.Fatalf("marshal ref2 failed: %v", err)
}

if !bytes.Equal(b1, b2) {
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

For the prototest JSON roundtrip, the test compares proto.Marshal output bytes (b1 vs b2). Protobuf binary encoding isn’t guaranteed to be byte-for-byte stable across implementations/options even when the decoded messages are equivalent, so this can create unnecessary brittleness. Consider asserting semantic equality instead (e.g., compare the decoded messages/fields) and reserve byte-level equality for the cross-type encoding check earlier in the fuzz.

Suggested change
if !bytes.Equal(b1, b2) {
if !proto.Equal(&ref2, &j2) {

Copilot uses AI. Check for mistakes.
Signed-off-by: chethanm99 <chethanm1399@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants