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

BLS tests: cover both implementations #2490

Merged
merged 105 commits into from
Aug 4, 2020

Conversation

Nashatyrev
Copy link
Contributor

PR Description

Follow up to #2453

Make all BLS*Test testing both Mikuli and Blst implementations

Honestly I don't like how this is implemented, so any JUnit hints on how to improve this solution are very appreciated

Documentation

  • I thought about documentation and added the documentation label to this PR if updates are required.

…aces

# Conflicts:
#	bls/src/main/java/tech/pegasys/teku/bls/BLS.java
#	bls/src/main/java/tech/pegasys/teku/bls/BLSPublicKey.java
#	bls/src/test/java/tech/pegasys/teku/bls/BLSPublicKeyTest.java
#	data/provider/src/main/java/tech/pegasys/teku/api/schema/BLSPubKey.java
# Conflicts:
#	bls/build.gradle
#	bls/src/test/java/tech/pegasys/teku/bls/impl/mikuli/hash2g2/ReferenceTests.java
…BLSPublicKey fromSSZBytes() and fromBytesCompressed() though they are equivalent with current SSZ implementation
…Compressed() though they are equivalent with the current SSZ implementation
…-bls-tests

# Conflicts:
#	bls/src/main/java/tech/pegasys/teku/bls/BLS.java
#	bls/src/test/java/tech/pegasys/teku/bls/BLSSecretKeyTest.java
#	bls/src/test/java/tech/pegasys/teku/bls/BLSTest.java
@Nashatyrev Nashatyrev changed the title Feature multi impl bls tests BLS tests: cover both implementations Jul 31, 2020
@mbaxter
Copy link
Contributor

mbaxter commented Aug 3, 2020

Honestly I don't like how this is implemented, so any JUnit hints on how to improve this solution are very appreciated

The only other way I can think to implement would be to use @ParameterizedTest to pass in different implementations, but I think that would require a bigger rework (moving away from static methods, etc). For example:

Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

LGTM

@ajsutton ajsutton merged commit 420b8b8 into Consensys:master Aug 4, 2020
@Nashatyrev Nashatyrev deleted the feature-multi-impl-bls-tests branch August 28, 2020 07:24
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