Skip to content

test: add comprehensive roundtrip tests and fix leading zeros bug - #55

Open
sumanjeet0012 wants to merge 3 commits into
multiformats:masterfrom
sumanjeet0012:fix-issue-40
Open

sumanjeet0012 wants to merge 3 commits into
multiformats:masterfrom
sumanjeet0012:fix-issue-40

Conversation

@sumanjeet0012

Copy link
Copy Markdown
Contributor

Fixes #40

Description

This pull request adds comprehensive round-trip tests with random data to verify that all encodings in py-multibase correctly handle various data shapes, including leading zero bytes and extreme sizes. In addition, it fixes a bug where BaseStringConverter was incorrectly dropping leading zeros upon encoding/decoding.

Changes

  • Created tests/test_roundtrip.py with random data generators covering all 24 supported encodings (e.g. base2, base8, base10, base16, base32z, base36, base58btc, etc.).
  • The tests check data integrity on edge-cases such as random sizes, all zeros (b'\x00\x00'), all ones (b'\xff\xff'), and leading zeroes (b'\x00hello').
  • Fixed BaseStringConverter.encode and decode in multibase/converters.py to correctly preserve leading zero bytes manually, as int.from_bytes naturally truncates them.
  • Fixed Base16StringConverter.decode in multibase/converters.py to use bytes.fromhex() instead of relying on the integer representation decoding.

@acul71 acul71 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.

Review (maintainer)

Thanks for the thorough round-trip coverage and the leading-zeros fix in BaseStringConverter / Base16StringConverter. This looks like the preferred fix relative to overlapping PR #48 (similar converter changes): #55 has green CI and broader tests aligned with #40.

Blocker

Missing newsfragment — please add newsfragments/40.bugfix.rst with a short user-facing note that leading zero bytes are preserved for integer-based multibase encodings, ending with a trailing newline. This is mandatory before approval.

Also note

  • Branch is 6 commits behind origin/master (dry-run merge is clean; please rebase/merge when convenient).
  • Minor: encode still shadows the builtin name bytes; Base16 decode comment still mentions case normalization after switching to bytes.fromhex.

Validation

Local make lint, typecheck, test (362 passed), and docs-ci all passed. GitHub checks are green.

Request changes until the newsfragment is added; happy to re-review promptly after that.

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.

No round-trip tests with random data and leading zeros

2 participants