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

Move arbitrary module into testutils module #1131

Closed
leighmcculloch opened this issue Nov 5, 2023 · 2 comments · Fixed by #1134
Closed

Move arbitrary module into testutils module #1131

leighmcculloch opened this issue Nov 5, 2023 · 2 comments · Fixed by #1134

Comments

@leighmcculloch
Copy link
Member

leighmcculloch commented Nov 5, 2023

The arbitrary module is only intended for use when testutils are enabled. We have most testutils organized under a testutils module. The arbitrary module seems to be the exception where it lives at the top-level.

Was there a reason we didn't want it to be inside testutils? @brson Do you remember?

If there's no reason, would anyone be opposed to moving it under testutils?

@brson
Copy link
Contributor

brson commented Nov 6, 2023

No reason. I just hadn't considered putting it there. I'm ok with moving it.

There's one it soroban-env-common as well. If this one moves, that one seemingly should too, though I don't think there is a corresponding testutils mod there. (edit: though the one in inv-common is not part of the public interface of that crate).

@brson
Copy link
Contributor

brson commented Nov 6, 2023

It may also be reasonable to hide the arbitrary module completely and just reexport the few things it defines publicly from testutils.

One of the things the arbitrary module reexports is the arbitrary crate, creating arbitrary::arbitrary paths, which is not so beautiful, and may or may not have been a good decision. Removing one level of arbitrary modules could be less ugly.

Edit: The arbitrary module does contain a bunch of mod-level docs, so removing it would require relocating them somewhere.

github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2023
### What

Move the arbitrary module under the testutils module.

FIxes #1131

### Why

All other testutils features are under the testutils module.

### Known limitations

This does not leave a deprecated module of reexports in the old
location, so fuzz users will experience breakage. The soroban-examples
repo and the fuzzing tutorial will need to be updated.

There is other possible restructuring of the arbitrary module that could
also be done, but is not, e.g. the contents of `arbitrary` could be
reexported directly under `testutils`.

This leaves the private `arbitrary_extra` module in its current
location.

`soroban-env-common` has a corresponding `arbitrary` module, but it is
private. Now these two crates will have their corresponding modules
located in different places.

---

I also updated the lockfile for tests/fuzz/fuzz, as Cargo seems to want
it updated.

---------

Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
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 a pull request may close this issue.

2 participants