Skip to content

Add unit tests for ByteArrayBuffer extension functions#251

Merged
zipdoki merged 3 commits into
kakao:mainfrom
kimsunho2000:test/byte-array-buffer-unit-tests
Apr 22, 2026
Merged

Add unit tests for ByteArrayBuffer extension functions#251
zipdoki merged 3 commits into
kakao:mainfrom
kimsunho2000:test/byte-array-buffer-unit-tests

Conversation

@kimsunho2000

@kimsunho2000 kimsunho2000 commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Add unit tests for ByteArrayBuffer extension functions defined in ByteArrayBuffer.kt.
Closes #119

Changes

  • Added ByteArrayBufferTest.kt at core/src/test/kotlin/com/kakao/actionbase/core/codec/

Test Plan

Test Coverage
putValue and getValue should round trip in ascending order putValue + getValue roundtrip with ASC ordering
putValue and getValue should round trip in descending order putValue + getValue roundtrip with DESC ordering
getValue should throw IllegalStateException when value is null getValue throws IllegalStateException when value is null
getValueOrNull should return null when value is null getValueOrNull returns null when value is null
hasRemaining should return true when data exists hasRemaining returns true when data exists
hasRemaining should return false after data is consumed hasRemaining returns false after data is consumed
hasRemaining should return false for empty buffer hasRemaining returns false for empty buffer
buffer should create ByteArrayBuffer from ByteArray buffer() creates valid ByteArrayBuffer from ByteArray
putValue should produce different bytes for asc and desc order ASC and DESC produce different byte representations
putValue and getValue should maintain order for multiple values putValue + getValue maintains insertion order for multiple values

How to Test

./gradlew spotlessApply
./gradlew :core:test

Copilot AI review requested due to automatic review settings April 19, 2026 12:07
@kimsunho2000
kimsunho2000 requested a review from em3s as a code owner April 19, 2026 12:07
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Apr 19, 2026
@CLAassistant

CLAassistant commented Apr 19, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds unit test coverage for ByteArrayBuffer extension functions in core to ensure serialization/deserialization and buffer helpers behave as expected (per #119).

Changes:

  • Introduced ByteArrayBufferTest covering putValue, getValue, getValueOrNull, hasRemaining, and ByteArray.buffer().
  • Added parameterized coverage across multiple primitive types and ordering modes (ASC/DESC).
  • Added assertions for null-handling behavior and ASC vs DESC byte differences.

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

Comment on lines +85 to +89
buffer.setPosition(0)

assertTrue(buffer.hasRemaining())
}

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

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

hasRemaining should return true when data exists is currently a vacuous assertion: after ByteArray(BUFFER_SIZE).buffer() and setPosition(0), remaining is still BUFFER_SIZE, so hasRemaining() would be true even if nothing had been written. Consider asserting on a read-buffer created from buffer.toByteArray().buffer() (so length reflects bytes actually written), and/or add a separate case like ByteArray(0).buffer() to verify hasRemaining() is false for an empty buffer.

Suggested change
buffer.setPosition(0)
assertTrue(buffer.hasRemaining())
}
val writtenBytes = buffer.toByteArray()
val readBuffer = writtenBytes.buffer()
assertTrue(readBuffer.hasRemaining())
}
@Test
fun `hasRemaining should return false for empty buffer`() {
val buffer = ByteArray(0).buffer()
assertFalse(buffer.hasRemaining())
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kimsunho2000
The current assertion is vacuous — since the buffer is backed by ByteArray(BUFFER_SIZE), remaining is always BUFFER_SIZE after setPosition(0) regardless of what was written. Consider switching to buffer.toByteArray().buffer() so the assertion actually reflects bytes written.

Also, if the intent was to cover the empty-buffer case, it might be worth adding a separate test like ByteArray(0).buffer() to explicitly verify hasRemaining() returns false.

@em3s

em3s commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

@zipdoki Please review when you get a chance.

@em3s
em3s requested review from zipdoki and removed request for em3s April 19, 2026 12:46
type: double
value: "3.14159"
"""
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kimsunho2000
nit: two small consistency things — indent the YAML to match the @ObjectSource style used in EdgeStateMapperTest, and add byte/short to cover the uncovered serialize paths.

@kimsunho2000

Copy link
Copy Markdown
Contributor Author

Thanks for the review! @zipdoki I'll update the code and push it soon.

@kimsunho2000

kimsunho2000 commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@zipdoki
I've addressed the review feedback:
commit: 13d391e

  • Fixed the vacuous hasRemaining assertion using buffer.toByteArray().buffer()
  • Added ByteArray(0).buffer() case to verify hasRemaining() returns false for empty buffer
  • Added byte and short types to FIELD_SOURCES
  • Fixed YAML indentation to match @ObjectSource style

Also, I added one more test beyond the issue requirements:
commit: 108b480

  • putValue and getValue should maintain order for multiple values — verifies that multiple values written sequentially are read back in the correct order, which reflects the actual usage pattern in EdgeIndexRecord.decodeKey.

Please let me know if there's anything else to improve. Thanks!

@zipdoki zipdoki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kimsunho2000
LGTM 👍

Thanks for your continued contributions!

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 22, 2026
@zipdoki
zipdoki merged commit 159ffa5 into kakao:main Apr 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests for ByteArrayBuffer extension functions

5 participants