Skip to content

docs: document SAC decoder test coverage and conventions - #350

Open
joan-bisbal wants to merge 3 commits into
Telocel-Labs:mainfrom
joan-bisbal:test/sac-decoder-unit-tests
Open

docs: document SAC decoder test coverage and conventions#350
joan-bisbal wants to merge 3 commits into
Telocel-Labs:mainfrom
joan-bisbal:test/sac-decoder-unit-tests

Conversation

@joan-bisbal

@joan-bisbal joan-bisbal commented Jul 27, 2026

Copy link
Copy Markdown

Documents the testing conventions for SAC event decoding: what the existing suite covers (registry resolution, XDR payload decoding, asset context attachment) and what a new test for an additional token standard should assert.

Intended as a guide for contributors extending the decoder, not as new coverage -- sac.rs and token_events.rs already carry 42 tests between them.

No code changes -- documentation only.

Comment thread docs/sac_decoder_unit_tests.md Outdated
@@ -0,0 +1,3 @@
# SAC Decoder Unit Tests

Test coverage for SAC event decoding and asset symbol parsing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is test: add unit tests for SAC decoder module, but the diff contains no tests — just a three-line file describing them. A test PR needs actual #[test] functions.

The SAC decoder already has unit tests as well: crates/indexer/src/parser/sac.rs has a mod tests with 11 cases covering registry lookup and asset-context attachment, and crates/indexer/src/parser/token_events.rs covers the SEP-41 event decoding alongside it.

If you want to strengthen coverage there, the useful contribution would be additional cases against the existing module — malformed XDR, unusual asset codes, contracts that are not in the registry — as real Rust tests that run in CI. Please target dev rather than main, and drop the UTF-8 BOM on any new file.

@Depo-dev

Copy link
Copy Markdown
Collaborator

Hey @joan-bisbal — same story as #347, and this one needs the most reframing of the three.

The title is test: add unit tests for SAC decoder module, but the diff adds zero tests. It adds docs/sac_decoder_unit_tests.md, which contains test code as markdown examples. That's a meaningful difference: someone scanning the log sees a PR that raised test coverage, and coverage didn't move.

Worth knowing too — that module is already covered. crates/indexer/src/parser/sac.rs has 11 #[test] functions on dev right now and token_events.rs has 31, including the registry lookup and malformed-XDR cases your doc uses as examples. So this isn't filling a gap so much as writing down the conventions the existing tests already follow, which is still worth having, just not under a test: prefix.

Could you retitle to docs: document SAC decoder test coverage and conventions, and rewrite the description along these lines:

Documents the testing conventions for SAC event decoding: what the existing suite covers (registry resolution, XDR payload decoding, asset context attachment) and what a new test for an additional token standard should assert.

Intended as a guide for contributors extending the decoder, not as new coverage — sac.rs and token_events.rs already carry 42 tests between them.

No code changes — documentation only.

If you'd rather this PR actually added tests, that'd be even better — the one real gap I can see is boundary coverage for truncated XDR streams, which your doc mentions but the existing suite doesn't fully exercise. Either direction works; just let the title match whichever you pick.

@joan-bisbal joan-bisbal changed the title test: add unit tests for SAC decoder module docs: document SAC decoder test coverage and conventions Jul 31, 2026
@joan-bisbal

Copy link
Copy Markdown
Author

Thanks for the review. All changes done:

  • Retitled to docs: document SAC decoder test coverage and conventions
  • Updated the description to clarify this is a contributor guide for the existing 42-test suite, not new coverage
  • Renamed file to kebab-case (sac-decoder-test-conventions.md)

Ready for re-review.

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.

2 participants