search

project-chip / rs-matter

Rust implementation of the Matter protocol. Status: Experimental
Apache License 2.0
326 stars 45 forks source link

Implement `FromTLV` for unit enums #143

Closed andy31415 closed 7 months ago

andy31415 commented 7 months ago

Changes

kedars commented 7 months ago

LGTM

Add a unit test file for TLV encode/decode since we seem to miss that (I had bugs in toTLV for example)

This is helpful.

There are also tests in rs-matter/src/tlv/traits.rs, but they also ensure that the TLVs are encoded/decoded as per the spec as well.

The current tests that you have added ensure that there is no information loss, and encode/decode are symmetric. This may be sufficient for most new data types that we introduce. I wonder if we should maintain all these in the same file, with some segregation so developers know which ones to add new test cases to.

andy31415 commented 7 months ago

LGTM

Add a unit test file for TLV encode/decode since we seem to miss that (I had bugs in toTLV for example)

This is helpful.

There are also tests in rs-matter/src/tlv/traits.rs, but they also ensure that the TLVs are encoded/decoded as per the spec as well.

The current tests that you have added ensure that there is no information loss, and encode/decode are symmetric. This may be sufficient for most new data types that we introduce. I wonder if we should maintain all these in the same file, with some segregation so developers know which ones to add new test cases to.

Let me know what you want to do ... I both saw traits.rs late and also wonder if these tests are move integration-level as they test macros as well as traits themselvs, so I could argue that separate location may be worth it.

I slightly prefer separation however I can also understand that having separate test location can be annoying. I would do whatever you think is best.