Skip to content

test(payloads): add encode/decode roundtrip tests for all payload types#95

Merged
phsym merged 1 commit intomainfrom
test/payload-roundtrip-coverage
Apr 22, 2026
Merged

test(payloads): add encode/decode roundtrip tests for all payload types#95
phsym merged 1 commit intomainfrom
test/payload-roundtrip-coverage

Conversation

@phsym
Copy link
Copy Markdown
Collaborator

@phsym phsym commented Apr 17, 2026

Summary

  • Replaces 3 individual roundtrip tests with a single table-driven TestPayloads_Encode_Decode covering all 27 request/response payload pairs
  • Every struct field is populated in the test data to ensure full encode/decode path coverage
  • Adds a normalizeTimesToUTC reflection helper to handle TTLV decoding time.Time in local timezone vs UTC originals

@phsym phsym requested a review from a team as a code owner April 17, 2026 09:13
@phsym phsym force-pushed the test/payload-roundtrip-coverage branch from 8a5992c to 9809295 Compare April 17, 2026 09:36
@phsym phsym requested a review from Copilot April 17, 2026 20:42
Copy link
Copy Markdown

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

Consolidates payload encode/decode roundtrip testing into a single table-driven test that aims to cover all request/response payload structs, with a reflection helper to normalize time.Time values for stable equality comparisons.

Changes:

  • Replaces multiple per-payload roundtrip tests with a single TestPayloads_Encode_Decode table-driven test covering many payload types.
  • Adds normalizeTimesToUTC reflection helper to convert decoded time.Time fields to UTC before equality checks.
  • Expands test fixtures to populate many fields across payload structs to exercise encode/decode paths.

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

Comment thread payloads/payloads_test.go
@phsym phsym force-pushed the test/payload-roundtrip-coverage branch from 9809295 to e3e5a70 Compare April 22, 2026 09:48
Only 3 payload types had roundtrip tests (Register request, Import request,
Export response). This adds table-driven tests covering all 27 request/response
payload pairs with all fields populated, bringing encode/decode coverage to
every payload struct in the package.

A normalizeTimesToUTC helper handles timezone differences in TTLV time decoding
so that reflect.DeepEqual works correctly regardless of the local timezone.

Signed-off-by: Pierre-Henri Symoneaux <pierre-henri.symoneaux@ovhcloud.com>
@phsym phsym force-pushed the test/payload-roundtrip-coverage branch from e3e5a70 to 9bd9900 Compare April 22, 2026 09:51
@phsym phsym requested a review from Copilot April 22, 2026 09:51
@phsym phsym merged commit 9a49944 into main Apr 22, 2026
8 checks passed
@phsym phsym deleted the test/payload-roundtrip-coverage branch April 22, 2026 09:54
Copy link
Copy Markdown

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 1 out of 1 changed files in this pull request and generated 3 comments.


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

Comment thread payloads/payloads_test.go
Comment on lines +49 to +55
for i := range v.NumField() {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := range v.Len() {
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

for i := range v.NumField() does not compile because range cannot be used on an int. Use an index loop (e.g., for i := 0; i < v.NumField(); i++) to iterate over struct fields.

Suggested change
for i := range v.NumField() {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := range v.Len() {
for i := 0; i < v.NumField(); i++ {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := 0; i < v.Len(); i++ {

Copilot uses AI. Check for mistakes.
Comment thread payloads/payloads_test.go
Comment on lines +49 to +55
for i := range v.NumField() {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := range v.Len() {
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

for i := range v.Len() does not compile because range cannot be used on an int. Use a standard index loop (e.g., for i := 0; i < v.Len(); i++) to iterate over slice elements.

Suggested change
for i := range v.NumField() {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := range v.Len() {
for i := 0; i < v.NumField(); i++ {
if f := v.Field(i); f.CanSet() {
normalizeTimesToUTC(f)
}
}
case reflect.Slice:
for i := 0; i < v.Len(); i++ {

Copilot uses AI. Check for mistakes.
Comment thread payloads/payloads_test.go
require.NoError(t, err)
require.EqualValues(t, req, decodedReq)
// normalizeTimesToUTC recursively converts all time.Time fields to UTC
// so that reflect.DeepEqual works regardless of timezone differences in TTLV decoding.
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

The comment says TTLV decoding yields time.Time in local timezone, but ttlv's binary DateTime decoding uses time.Unix(...) (which returns UTC). Since this test only exercises TTLV encode/decode, the justification for normalizeTimesToUTC appears inaccurate; either update the comment to match the actual behavior or scope/rename the helper to the encoding(s) that return local times.

Suggested change
// so that reflect.DeepEqual works regardless of timezone differences in TTLV decoding.
// so that reflect.DeepEqual compares equivalent instants consistently even
// when time values carry different location information.

Copilot uses AI. Check for mistakes.
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.

3 participants