Repository navigation
Conversation
|
Thanks for the PR. It is labeled If AI was used to create this PR, you must run the Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
Thanks @p4rtridge, and for picking up access_token.go from #2883. I checked the expected bytes against the three encoders in access_token.go and the command codes against core/binary_protocol/src/codes.rs (41, 42, 43), and they match. One small thing: the old failure message printed the bodies with |
Thanks for reviewing! I’ve changed the failure message from |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4640 +/- ##
=============================================
- Coverage 90.86% 80.71% -10.16%
- Complexity 3104 3107 +3
=============================================
Files 1479 1467 -12
Lines 361309 356151 -5158
Branches 305179 300040 -5139
=============================================
- Hits 328296 287453 -40843
- Misses 26111 61873 +35762
+ Partials 6902 6825 -77
🚀 New features to boost your workflow:
|
|
/skill team-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: The new access-token serialization tests match the command codes, byte layout, and server wire format. The added name rows miss the 255-byte boundary, the empty-name rows encode a rejected name, and the three tests duplicate one harness.
Counts: critical 0, warning 0, nit 2, simplification 1
This review was generated by Claude Code 2.1.310 on deepseek-flash[1m]. Review the output before you act on it.
| }, | ||
| { | ||
| name: "empty name", | ||
| request: &CreatePersonalAccessToken{Name: "", Expiry: 0x01020304}, |
There was a problem hiding this comment.
nit: The empty name rows assert a body the server rejects, because WireName requires 1 to 255 bytes (core/binary_protocol/src/primitives/identifier.rs:105). Replace them with a one-character name, the shortest name WireName accepts. Also at line 130.
| }, | ||
| { | ||
| name: "longer name", | ||
| request: &CreatePersonalAccessToken{Name: "my-access-token", Expiry: iggcon.IggyExpiryServerDefault}, |
There was a problem hiding this comment.
nit: The name rows stop at 15 bytes, so the 255-byte wire limit and the byte(len(name)) wrap above 255 stay untested (foreign/go/internal/command/access_token.go:38). Add a 255-byte case and a case above 255 that expects an error.
| } | ||
|
|
||
| for _, test := range tests { | ||
| func TestGetPersonalAccessTokensMarshalBinary(t *testing.T) { |
There was a problem hiding this comment.
simplification: The three new tests repeat the same table, code check, and marshal loop. Fold them into one table over Command, as TestPartitionCommandsMarshalBinary does at foreign/go/internal/command/partition_test.go:27, to remove about 50 lines.
Which issue does this PR address?
Relates to #2883
Rationale
The personal access token command encoders in the Go SDK had only partial
serialization coverage. The existing test checked the
CreatePersonalAccessTokenexpiry encoding with a single fixed name. It did not check the command code,
the name length prefix for other name lengths, or the other two access token
commands. Exact-byte assertions catch protocol compatibility regressions.
What changed?
Reworked
access_token_test.gointo table-driven serialization tests for allthree commands in
access_token.go. Each case checks the command code and thecomplete request body.
CreatePersonalAccessToken(code 42): the four existing expiry cases (serverdefault, little-endian layout, values past 32 bits, never expire), plus an
empty name and a longer name, to cover the
u8length prefix.GetPersonalAccessTokens(code 41): the body is empty.DeletePersonalAccessToken(code 43): short, empty and longer names.That is 10 cases. No production code changed.
Local Execution
go test -count=1 ./...,go test -race -count=1 ./...andgo vet ./...fromforeign/go.AI Usage
against the encoders in
access_token.goand the server's protocoldefinitions.