Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: verifreg: add initial cbor encoding forms tests #263

Merged
merged 3 commits into from
May 29, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 7, 2024

Since I was touching type changes in #262 I thought I'd do an initial version of something I've proposed doing a more comprehensive form of: cbor encoding and round-trip testing of publicly exported types, which is particularly important for the types we pass through builtin-actors. So for now I'm just starting with ClaimAllocationsParams, and am following this up with a PR to builtin-actors with identical fixtures.

Ideally we'd expand this, perhaps slowly, to cover the full suite of types that pass through this barrier, and in the process provide fixtures for other implementations to test against as well.

The diagnostic output might be overkill, I can remove that if someone wants to object, I just like being absolutely clear.

@rvagg
Copy link
Member Author

rvagg commented May 7, 2024

Rust version: filecoin-project/builtin-actors#1538

@rvagg rvagg force-pushed the rvagg/verifreg-encoding-tests branch from cba8a34 to e744dea Compare May 7, 2024 04:49
Base automatically changed from rvagg/sector-allocation-claims to master May 24, 2024 01:34
Starting with ClaimAllocationsParams for now, matching with Rust versions of
this.
@rvagg rvagg force-pushed the rvagg/verifreg-encoding-tests branch from e744dea to bf4c8cd Compare May 24, 2024 02:05
@rvagg
Copy link
Member Author

rvagg commented May 24, 2024

removed diagnostic output, the more I look at it the more wasteful it seems when we expand the number of these

@rvagg rvagg force-pushed the rvagg/verifreg-encoding-tests branch from bf4c8cd to 77ba903 Compare May 24, 2024 04:38
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

It might be overkill to do this for multiple versions of a type when nothing changes.

If we could remove these from next to the code and put them in some place where all versions are accessible we could maybe only update tests when the actual types change. That would reduce code volume a lot for the same value. Idk though it's pretty cheap to copy paste a test and carry all these along.

builtin/v13/verifreg/verifreg_types_test.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented May 29, 2024

Regarding duplication - my theory here is that as we start to accumulate these, a pattern should emerge that can be extracted to make this simpler.

Perhaps reviving https://github.com/filecoin-project/test-vectors at some point for these would be good and then run the tests from both ends. For IPLD I set up this: https://github.com/ipld/codec-fixtures which runs a set of fixtures across the latest codec implementations of Go, JS and Rust each night and complains if there are problems. I think a similar pattern could work here, but I don't really want to dive in too deep right now.

@rvagg rvagg merged commit 6568009 into master May 29, 2024
5 checks passed
@rvagg rvagg deleted the rvagg/verifreg-encoding-tests branch May 29, 2024 05:07
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.

None yet

2 participants