feat(go): add comprehensive unit tests for command serialization by saie-ch · Pull Request #2973 · apache/iggy

saie-ch · 2026-03-18T15:24:00Z

Which issue does this PR close?

Rationale

The Go SDK contains numerous command types that implement the MarshalBinary interface for binary serialization, but many lacked comprehensive unit tests. Without thorough testing, serialization bugs can cause data corruption, runtime panics, and wire protocol incompatibility with the Iggy server and other SDKs.

What changed?

Before: Go SDK had only 6 basic tests for command serialization.

After: Added 75 new comprehensive unit tests (81 total) covering 36 commands with:

Bugs Fixed:

Permissions.MarshalBinary() - Fixed first permission byte corruption when streams are nil
CreateUser - Fixed off-by-one buffer allocation (extra trailing byte)
UpdatePermissions - Fixed panic when permissions are nil (missing flag byte allocation)
UpdateUser - Fixed panic when both username and status are nil (insufficient buffer size)

Test Coverage:

Overall package: 75.9% coverage
Tested commands: 85-100% coverage per command
All tests pass with race detector enabled

Local Execution

All 81 tests passed
Pre-commit hooks ran (format, tidy, generate check, vet, build)
Race detector enabled - no race conditions detected
Coverage: 75.9% in internal/command package
All pre-merge checks passed:

AI Usage

Tool: Claude Code (claude-sonnet-4.5)

Scope: Test implementation, bug discovery, and cross-SDK validation

Helped design comprehensive test coverage strategy
Discovered 4 serialization bugs through systematic edge case testing
Generated test structures following Go best practices
Assisted with cross-validation against Java and Rust SDK implementations
Provided guidance on fixing buffer allocation bugs

Verification:

All tests run successfully with race detector enabled
Compared implementation patterns with Rust SDK reference implementation

codecov · 2026-03-18T15:38:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.49%. Comparing base (c6a404a) to head (6319a42).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2973      +/-   ##
============================================
+ Coverage     74.47%   74.49%   +0.02%     
  Complexity      943      943              
============================================
  Files          1188     1188              
  Lines        106530   106530              
  Branches      83561    83561              
============================================
+ Hits          79333    79360      +27     
+ Misses        24448    24425      -23     
+ Partials       2749     2745       -4

Components	Coverage Δ
Rust Core	`75.74% <ø> (+0.02%)`	⬆️
Java SDK	`60.14% <ø> (ø)`
C# SDK	`69.38% <ø> (-0.04%)`	⬇️
Python SDK	`81.43% <ø> (ø)`
Node SDK	`91.41% <ø> (-0.13%)`	⬇️
Go SDK	`40.24% <100.00%> (+0.43%)`	⬆️

Files with missing lines	Coverage Δ
foreign/go/internal/command/update_user.go	`95.12% <100.00%> (+41.46%)`	⬆️
foreign/go/internal/command/user.go	`91.30% <100.00%> (ø)`

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

atharvalade

overall looking good but I found a few things that need to be addressed

atharvalade · 2026-03-19T09:08:53Z

I found 3 pre-existing issues #2980 #2981 #2982
I will wait for this merge and @saie-ch's comments before starting work on the issues mentioned above.

hubcio · 2026-03-19T09:44:39Z

@chengxilo could you please also check this?

chengxilo · 2026-03-19T19:12:27Z

This part should not use byte(0) directly, doesn't align with the behavior in rust sdk.

iggy/core/common/src/types/permissions/permissions_global.rs

Lines 267 to 273 in 9c8b5cc

if current_stream < streams_count {

current_stream += 1;

bytes.put_u8(1);

} else {

bytes.put_u8(0);

}

}

@saie-ch would you mind to solve this in another PR first (to Implement unit test for permission and fix the problem in permission)?

chengxilo · 2026-03-19T19:53:45Z

@@ -43,14 +43,14 @@ func (u *UpdateUser) MarshalBinary() ([]byte, error) {
 	username := *u.Username


Here the u.Username is actually modified by the method, I don't think it's a good approach. Since you already modifed this method, would you mind to modified this too?

chengxilo · 2026-03-19T19:56:22Z

regarding #2973 (comment), I think it would be great to write tests to check whether they are modified after call MarshalBinary, for each command.

saie-ch · 2026-03-20T02:26:09Z

Thanks @atharvalade and @chengxilo, I will implement the necessary changes.

hubcio · 2026-03-26T11:14:56Z

@saie-ch @chengxilo what's the status of this PR? is it ready to be merged? should #3015 be merged first?

saie-ch · 2026-03-26T11:59:03Z

@saie-ch @chengxilo what's the status of this PR? is it ready to be merged? should #3015 be merged first?

Yes please @hubcio, let's wait for that to be merged first. I just pushed new changes.

chengxilo · 2026-03-26T15:08:08Z

This PR may still need some further review and improvement. We should merge #3015 first tho.

hubcio · 2026-03-30T07:08:19Z

#3015 is merged.

hubcio · 2026-03-30T09:00:20Z

@saie-ch @atharvalade what is the difference between this PR and #3037 ?

chengxilo · 2026-03-30T12:48:40Z

@saie-ch @atharvalade what is the difference between this PR and #3037 ?

This one is created a few days earlier and only covers the test for commands.🤕

github-actions · 2026-04-12T00:47:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.

If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.

Thank you for your contribution!

…fix 4 bugs

github-actions · 2026-04-25T00:45:39Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.

If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.

Thank you for your contribution!

atharvalade · 2026-04-29T16:44:23Z

@saie-ch do you plan to implement anything else now that #3015 is merged? Once I get a green light from you, I can start review again and plan to merge this too after reviews. BTW I closed #3037 because of multiple conflicts and outdated code.

saie-ch · 2026-04-29T19:09:25Z

@saie-ch do you plan to implement anything else now that #3015 is merged? Once I get a green light from you, I can start review again and plan to merge this too after reviews. BTW I closed #3037 because of multiple conflicts and outdated code.

Yes, please review. I believe, i've implemented the necessary changes.

github-actions · 2026-05-08T00:54:04Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.

If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.

Thank you for your contribution!

hubcio · 2026-05-14T11:55:45Z

/ready

hubcio · 2026-05-14T14:42:36Z

@saie-ch I noticed that comments from this PR are not fixed (output from claude code):

  #: 2958694591
  Reviewer: atharvalade
  Location: access_token.go:36
  Ask: Expiry field → uint64, use PutUint64 (current PutUint32 writes expiry into high 32 bits → server reads expiry << 32)
  Status: ❌ NOT fixed — file never touched. Line 24 still uint32, line 36 still PutUint32
  ────────────────────────────────────────
  #: 2958698450
  Reviewer: atharvalade
  Location: user.go:77 (CreateUser)
  Ask: nil-permissions branch must also write permissions_len:u32_le=0
  Status: ❌ NOT fixed — else branch still writes only 1 flag byte; capacity has no +4 for nil case
  ────────────────────────────────────────
  #: 2958701940
  Reviewer: atharvalade
  Location: user.go UpdatePermissions
  Ask: base length = len(userIdBytes)+1+4; else branch must write 4 zero bytes
  Status: ❌ NOT fixed — line 119 still len(userIdBytes)+1, else branch line 142 still single 0 byte. Code untouched
  ────────────────────────────────────────
  #: 2962372305
  Reviewer: chengxilo
  Location: update_user.go:43
  Ask: stop mutating receiver u.Username
  Status: ❌ NOT fixed — lines 39-43 still assign u.Username = new(string) then deref

can you fix them?

/author

saie-ch · 2026-05-14T15:27:45Z

@saie-ch I noticed that comments from this PR are not fixed (output from claude code):

  #: 2958694591
  Reviewer: atharvalade
  Location: access_token.go:36
  Ask: Expiry field → uint64, use PutUint64 (current PutUint32 writes expiry into high 32 bits → server reads expiry << 32)
  Status: ❌ NOT fixed — file never touched. Line 24 still uint32, line 36 still PutUint32
  ────────────────────────────────────────
  #: 2958698450
  Reviewer: atharvalade
  Location: user.go:77 (CreateUser)
  Ask: nil-permissions branch must also write permissions_len:u32_le=0
  Status: ❌ NOT fixed — else branch still writes only 1 flag byte; capacity has no +4 for nil case
  ────────────────────────────────────────
  #: 2958701940
  Reviewer: atharvalade
  Location: user.go UpdatePermissions
  Ask: base length = len(userIdBytes)+1+4; else branch must write 4 zero bytes
  Status: ❌ NOT fixed — line 119 still len(userIdBytes)+1, else branch line 142 still single 0 byte. Code untouched
  ────────────────────────────────────────
  #: 2962372305
  Reviewer: chengxilo
  Location: update_user.go:43
  Ask: stop mutating receiver u.Username
  Status: ❌ NOT fixed — lines 39-43 still assign u.Username = new(string) then deref

can you fix them?

/author

thanks @hubcio , will try to fix them.

atharvalade suggested changes Mar 19, 2026

View reviewed changes

Comment thread foreign/go/internal/command/access_token_test.go

Comment thread foreign/go/internal/command/user_test.go

Comment thread foreign/go/internal/command/user_test.go

atharvalade mentioned this pull request Mar 19, 2026

Go SDK: UpdatePermissions omits mandatory permissions_len field when permissions are nil #2982

Open

chengxilo reviewed Mar 19, 2026

View reviewed changes

saie-ch mentioned this pull request Mar 23, 2026

fix(go): correct Permissions serialization and add unit tests #3015

Merged

chengxilo mentioned this pull request Mar 27, 2026

test(go): increase Go SDK Codecov coverage with comprehensive tests #3037

Closed

saie-ch force-pushed the go_unit_tests branch from 4fa2b67 to 8b59b37 Compare April 4, 2026 15:35

github-actions Bot added stale Inactive issue or pull request and removed stale Inactive issue or pull request labels Apr 12, 2026

feat(go): add comprehensive unit tests for command serialization and …

f476720

…fix 4 bugs

saie-ch force-pushed the go_unit_tests branch from 8b59b37 to f476720 Compare April 17, 2026 18:42

github-actions Bot added stale Inactive issue or pull request and removed stale Inactive issue or pull request labels Apr 25, 2026

github-actions Bot added the stale Inactive issue or pull request label May 8, 2026

github-actions Bot removed the stale Inactive issue or pull request label May 9, 2026

Merge branch 'master' into go_unit_tests

6319a42

github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label May 14, 2026

github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels May 14, 2026

	if current_stream < streams_count {
	current_stream += 1;
	bytes.put_u8(1);
	} else {
	bytes.put_u8(0);
	}
	}

		@@ -43,14 +43,14 @@ func (u *UpdateUser) MarshalBinary() ([]byte, error) {
		username := *u.Username

Conversation

saie-ch commented Mar 18, 2026

Which issue does this PR close?

Rationale

What changed?

Local Execution

AI Usage

Uh oh!

codecov Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

atharvalade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atharvalade commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hubcio commented Mar 19, 2026

Uh oh!

chengxilo Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chengxilo Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengxilo commented Mar 19, 2026

Uh oh!

saie-ch commented Mar 20, 2026

Uh oh!

hubcio commented Mar 26, 2026

Uh oh!

saie-ch commented Mar 26, 2026

Uh oh!

chengxilo commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hubcio commented Mar 30, 2026

Uh oh!

hubcio commented Mar 30, 2026

Uh oh!

chengxilo commented Mar 30, 2026

Uh oh!

github-actions Bot commented Apr 12, 2026

Uh oh!

github-actions Bot commented Apr 25, 2026

Uh oh!

atharvalade commented Apr 29, 2026

Uh oh!

saie-ch commented Apr 29, 2026

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

hubcio commented May 14, 2026

Uh oh!

hubcio commented May 14, 2026

Uh oh!

saie-ch commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Mar 18, 2026 •

edited

Loading

atharvalade commented Mar 19, 2026 •

edited

Loading

chengxilo Mar 19, 2026 •

edited

Loading

chengxilo commented Mar 26, 2026 •

edited

Loading