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: Move Protobuf conformance test #35

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Nov 15, 2023

What ❔

Moves the Protobuf conformance test binary to an example in the zksync_protobuf crate.

Why ❔

zksync_protobuf relies on some "heavy" dependencies (e.g., Tokio, tracing-subscriber) just to implement the conformance test.

@slowli slowli marked this pull request as ready for review November 15, 2023 11:13
@slowli
Copy link
Contributor Author

slowli commented Nov 15, 2023

There are a couple of other options which would have the same effect:

  • Making conformance_test an example in the zksync_protobuf crate instead of a binary target. This would allow to have additional deps it brings as dev dependencies (i.e., they would not be included in the dependency graph for crates depending on zksync_protobuf).
  • Gating conformance_test binary behind an opt-in feature. This looks kind of awkward since IMO conformance_test is not an integral part of zksync_protobuf (it's, well, a test).

@pompon0
Copy link
Collaborator

pompon0 commented Nov 15, 2023

I'd prefer to put it to examples then (assuming this doesn't have any nasty side-effects) as this is a test of this specific crate. IMO moving it to a crate with random stuff detaches it too much from the code under test.

@brunoffranca brunoffranca merged commit ed71b2e into main Nov 16, 2023
4 checks passed
@brunoffranca brunoffranca deleted the aov-bft-379-move-protobuf-conformance-test branch November 16, 2023 19:27
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.

3 participants