Add tests for Option `toString` #340

zalenskivolt · 2025-03-22T23:26:25Z

No description provided.

codecov · 2025-03-24T18:45:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.74%. Comparing base (e70a0a5) to head (65261d1).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
+ Coverage   64.81%   69.74%   +4.93%     
==========================================
  Files           9        9              
  Lines         790      790              
  Branches      176      176              
==========================================
+ Hits          512      551      +39     
+ Misses        218      171      -47     
- Partials       60       68       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

zalenski · 2025-03-24T22:16:11Z

(moved new options in comments to zalenskivolt#2
will update this as options gets added, or update option PRs if this gets added…)

twyatt · 2025-03-25T19:45:58Z

I'm reluctant to add tests around toString (despite you catching / fixing some small issues). As we don't / shouldn't provide any guarantees around the format of toString output — it is intended for debugging/logging.

Tests just add an unnecessary maintenance burden when making changes to the output format of toString. Although my statement loses a bit of weight in the wake of you finding bugs in some of the toString outputs. 🙈

Definitely open to changing / fixing some of the toString outputs, I'm just hesitant to add units tests around it.

As for the serialization support (via kotlinx serialization), your tests are exposing a lot of gaps in that support. It looks like it could use a lot of love / fixing. I feel that effort should be broken out into a separate PR, as it may warrant a bit of iteration / discussion.

zalenski · 2025-03-26T00:16:53Z

I'm reluctant to add tests around toString (despite you catching / fixing some small issues). As we don't / shouldn't provide any guarantees around the format of toString output — it is intended for debugging/logging.

I've updated to only test things that are actually overridden with custom code, for a lot fewer tests.
I do think those should be tested for functionality, even if there are no guarantees around them.

Tests just add an unnecessary maintenance burden when making changes to the output format of toString. Although my statement loses a bit of weight in the wake of you finding bugs in some of the toString outputs. 🙈

now there are a lot fewer tests, and they are also not needlessly repeated for the serialization, so I think the maintenance burden is minimal. you'd only have to update if something actually changes.

zalenski · 2025-03-26T00:18:20Z

Definitely open to changing / fixing some of the toString outputs, I'm just hesitant to add units tests around it.

those are currently also fixed in the Oscore PR, where I found them by mistake when copy-pasting one of them.

I think most code warrant testing, and also dislike when there are obstacles to testing things (for example side effects like logging, time, file&network operations, database code, etc.) how well I follow that in general is another matter.

As for the serialization support (via kotlinx serialization), your tests are exposing a lot of gaps in that support. It looks like it could use a lot of love / fixing. I feel that effort should be broken out into a separate PR, as it may warrant a bit of iteration / discussion.

What is the wanted format for those? Should all HTTP-like options look like HTTP headers? And CoAP specific things as now, or also in a header format?

That could definitely be its own PR, with all serialization tests updated in it! 😉

zalenski · 2025-03-26T00:22:07Z

koap-core/src/commonTest/kotlin/MessageTest.kt

+        // observe
+        assertToString(Observe(Register), "Observe(value=0/Register)")
+        assertToString(Observe(Deregister), "Observe(value=1/Deregister)")
+        assertToString(Observe(1234567), "Observe(value=1234567)")


I think 0 and 1 could be sent in a notification response as a serial number, so they are not necessarily Register/Deregister requests. But maybe nice to see registration requests written out?

or an update for another PR?

Ya, I think it warrants being pulled out of this PR, only because I'm pretty torn on the best approach, so seems like we could discuss further rather than holding this PR up.

moved to #342

zalenskivolt · 2025-03-26T01:16:22Z

What is the wanted format for those? Should all HTTP-like options look like HTTP headers? And CoAP specific things as now, or also in a header format?

Serialize more options like http headers #341

twyatt · 2025-03-28T01:03:11Z

koap-core/src/commonTest/kotlin/MessageTest.kt

+        assertToString(Accept(ContentFormat.JSON), "Accept(JSON)")
+
+        // hex
+        assertToString(ETag("123".encodeToByteArray()), "ETag(etag=31 32 33)")


For objects that only have a single parameter, I think it makes sense to omit it from the toString.

Suggested change

assertToString(ETag("123".encodeToByteArray()), "ETag(etag=31 32 33)")

assertToString(ETag("123".encodeToByteArray()), "ETag(31 32 33)")

For objects that only have a single parameter, I think it makes sense to omit it from the toString.

in that case, do you want to override all one-parameter data class toString methods, since the format is based on those? there's around 12 options with a single parameter using the default implementation atm.

many of these have a http-like serialization suggested in #341 anyway, so I'm not sure of the benefit in making special toString for all of them. my suggestion would be to leave the toString format to look like a data class, with just the overrides for hex strings as needed?

twyatt · 2025-03-28T01:08:00Z

koap-core/src/commonTest/kotlin/MessageTest.kt

+        assertToString(ETag("123".encodeToByteArray()), "ETag(etag=31 32 33)")
+        assertToString(IfMatch("123".encodeToByteArray()), "IfMatch(etag=31 32 33)")
+        assertToString(IfMatch("".encodeToByteArray()), "IfMatch(etag=)")
+        assertToString(Echo("echo".encodeToByteArray()), "Echo(value=65 63 68 6F)")


It may be easier to copy/paste if the hex format was w/o spaces.
The default HexFormat is lowercase as well, I think that is sensible and we should probably update to align with that.
Perhaps an 0x prefix would be helpful to identify it as hex while still making copy/paste easy.

Suggested change

assertToString(Echo("echo".encodeToByteArray()), "Echo(value=65 63 68 6F)")

assertToString(Echo("echo".encodeToByteArray()), "Echo(0x6563686f)")

this just uses ByteArray.toHexString() from Debug.kt.
in #341, it actually uses @ExperimentalStdlibApi, which also implements toHexString() but without spaces, which I also think might be better here.

default HexFormat is lowercase as well

I've come to like uppercase hex more and more for readability. from old c there's no real default, since you choose %x or %X in printf:s yourself… 😄

I'll let you decide how to resolve this, with the Debug.kt method using the same name as the new stdlib api. should Debug.kt be updated to something like toHexStringUppercaseWithSpaces or toDebugHexString?
I think that version is still wanted for formatting message payloads.

The new HexFormat can do both byteSeparator = " ", upperCase = true or false and prefix = "0x", so maybe @ExperimentalStdlibApi is good to use for all of Message.kt and EncoderTest.kt where Debug.kt is used now?

twyatt · 2025-03-28T01:09:07Z

koap-core/src/commonTest/kotlin/MessageTest.kt

 import kotlin.test.Test
 import kotlin.test.assertEquals

 class MessageTest {

+    @Test
+    fun optionToStringOverrides() {
+        // content format


IMHO, these comments don't add any value.
If for organization, I think separate test functions would be a better alternative (e.g. fun contentFormatOptionsToString() { .. }).

Suggested change

// content format

twyatt · 2025-03-28T01:12:02Z

koap-core/src/commonTest/kotlin/MessageTest.kt

+        // observe
+        assertToString(Observe(Register), "Observe(value=0/Register)")
+        assertToString(Observe(Deregister), "Observe(value=1/Deregister)")
+        assertToString(Observe(1234567), "Observe(value=1234567)")


Ya, I think it warrants being pulled out of this PR, only because I'm pretty torn on the best approach, so seems like we could discuss further rather than holding this PR up.

twyatt · 2025-03-28T01:13:26Z

Left some comments re: changing the toString implementations. If preferred, I'm fine w/ deferring those changes if we just want to get these tests in and then make changes in other PRs.

zalenski · 2025-03-31T20:24:28Z

Left some comments re: changing the toString implementations. If preferred, I'm fine w/ deferring those changes if we just want to get these tests in and then make changes in other PRs.

I think I'd prefer that! 😄

This reverts commit a0f9a5f.

zalenski · 2025-04-01T11:16:18Z

webapp/src/jsTest/kotlin/serialization/SerializerTest.kt

-class SerializerTest {
-    @Test
-    fun optionSerializesTo() {
-        assertOptionSerializesTo(ContentFormat.PlainText, "Content-Format: text/plain; charset=utf-8")


moving serializer test to #341 only, where plenty of more special serializations are added

zalenski · 2025-04-01T11:21:01Z

Left some comments re: changing the toString implementations. If preferred, I'm fine w/ deferring those changes if we just want to get these tests in and then make changes in other PRs.

I think I'd prefer that! 😄

Updated this PR to only test Option toString overrides, just like methods and responses are tested. Moved all serialization tests to #341 where a lot more of those are added. No extra comments, no changes to implementations in this PR…

So please consider if you're ok with testing the code 😄

twyatt

I'd like to tweak the output of these toString implementations, but as discussed, that can be saved for a later PR. For now, this looks great!

zalenskivolt requested review from twyatt and a team as code owners March 22, 2025 23:26

zalenskivolt requested a review from cedrickcooke March 22, 2025 23:26

zalenskivolt force-pushed the test-option-to-string branch from 54de636 to 418c88c Compare March 22, 2025 23:29

zalenskivolt mentioned this pull request Mar 22, 2025

Add NoResponse option #332

Merged

cedrickcooke approved these changes Mar 24, 2025

View reviewed changes

zalenskivolt force-pushed the test-option-to-string branch 3 times, most recently from e283ee3 to 91c5501 Compare March 24, 2025 22:13

zalenskivolt force-pushed the test-option-to-string branch 2 times, most recently from 0d774c1 to c5c4a4a Compare March 25, 2025 19:18

zalenskivolt force-pushed the test-option-to-string branch 6 times, most recently from b34b1dc to 4b8ca0e Compare March 25, 2025 23:57

zalenski reviewed Mar 26, 2025

View reviewed changes

twyatt reviewed Mar 28, 2025

View reviewed changes

zalenskivolt force-pushed the test-option-to-string branch 4 times, most recently from 988de4c to c32fe8c Compare March 31, 2025 14:29

zalenskivolt force-pushed the test-option-to-string branch from b768e94 to 0c618be Compare March 31, 2025 19:32

Add tests for option toString overrides

7d32e44

zalenskivolt force-pushed the test-option-to-string branch from 59f2efc to d0f4f68 Compare March 31, 2025 20:28

Add tests for OptionSerializer special serialization

a0f9a5f

zalenskivolt force-pushed the test-option-to-string branch from d0f4f68 to a0f9a5f Compare April 1, 2025 09:07

Revert "Add tests for OptionSerializer special serialization"

65261d1

This reverts commit a0f9a5f.

zalenski reviewed Apr 1, 2025

View reviewed changes

zalenskivolt changed the title ~~Add tests for option toString and OptionSerializer~~ Add tests for Option toString Apr 1, 2025

twyatt approved these changes Apr 3, 2025

View reviewed changes

twyatt added maintenance General maintenance that doesn't effect the public API. patch Changes that should bump the PATCH version number and removed maintenance General maintenance that doesn't effect the public API. labels Apr 3, 2025

twyatt merged commit e426dfd into JuulLabs:main Apr 3, 2025
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for Option `toString` #340

Add tests for Option `toString` #340

zalenskivolt commented Mar 22, 2025

codecov bot commented Mar 24, 2025 •

edited

Loading

zalenski commented Mar 24, 2025

twyatt commented Mar 25, 2025

zalenski commented Mar 26, 2025

zalenski commented Mar 26, 2025

zalenski Mar 26, 2025

zalenski Mar 26, 2025

twyatt Mar 28, 2025

zalenski Mar 31, 2025

zalenskivolt commented Mar 26, 2025

twyatt Mar 28, 2025

zalenski Mar 31, 2025

twyatt Mar 28, 2025

zalenski Mar 31, 2025

zalenski Mar 31, 2025

twyatt Mar 28, 2025

zalenski Mar 31, 2025

twyatt Mar 28, 2025

twyatt commented Mar 28, 2025

zalenski commented Mar 31, 2025

zalenski Apr 1, 2025

zalenski commented Apr 1, 2025

twyatt left a comment

	assertToString(ETag("123".encodeToByteArray()), "ETag(etag=31 32 33)")
	assertToString(ETag("123".encodeToByteArray()), "ETag(31 32 33)")

	assertToString(Echo("echo".encodeToByteArray()), "Echo(value=65 63 68 6F)")
	assertToString(Echo("echo".encodeToByteArray()), "Echo(0x6563686f)")

Add tests for Option toString #340

Add tests for Option toString #340

Conversation

zalenskivolt commented Mar 22, 2025

codecov bot commented Mar 24, 2025 • edited Loading

Codecov Report

zalenski commented Mar 24, 2025

twyatt commented Mar 25, 2025

zalenski commented Mar 26, 2025

zalenski commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalenskivolt commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt commented Mar 28, 2025

zalenski commented Mar 31, 2025

Choose a reason for hiding this comment

zalenski commented Apr 1, 2025

twyatt left a comment

Choose a reason for hiding this comment

Add tests for Option `toString` #340

Add tests for Option `toString` #340

codecov bot commented Mar 24, 2025 •

edited

Loading