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

Added Ed25519 tests #54

Merged
merged 3 commits into from
Feb 24, 2021
Merged

Added Ed25519 tests #54

merged 3 commits into from
Feb 24, 2021

Conversation

melatron
Copy link
Contributor

Description of change

  • Added Malleability test case.
  • Added zip215 tests.
  • Added ed25519 sign.input tests.
  • Removed TODO that is not needed.
  • Fixes issue Ed25519: Improve tests #19

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How the change has been tested

All existing and new tests pass.

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

* Added Malleability test case.
* Added zip215 tests.
* Added ed25519 sign.input tests.
* Removed TODO that is not needed.
@nothingismagick
Copy link
Contributor

Copying @Wollac's comment from previous PR
image

Copy link
Contributor

@nothingismagick nothingismagick left a comment

Choose a reason for hiding this comment

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

Please fix Clippy (even though it isn't your fault, its because of rust 1.50.0) - this is open source gardening :D

Clippy fixed here: https://github.com/iotaledger/crypto.rs/pull/52/files#diff-eb22024a611ef759fe3cbefcf751bf535d92d3154fc0e6641944cda7a82f654b

and also add a changefile:
https://github.com/iotaledger/crypto.rs/tree/dev/.changes

@nothingismagick
Copy link
Contributor

For the reviewers: I have spoken with @melatron and he will setup gpg signing for his next PRs. I will sign off for this PR.

@melatron
Copy link
Contributor Author

@Wollac About your questions from - #53 (comment)

The idea of leaving the tests there is that in Rust you write integration tests in the /tests folder and unit tests under the file for the specific functionality you are testing. For the tests that are in src/ed25519.rs they also use crate::test_utils which is not listed as a feature in the Cargo.toml file. I guess the idea is to be used only internally. Because of that I figured that these tests are more like unit tests with internal use of test_utils.

@Wollac
Copy link
Contributor

Wollac commented Feb 17, 2021

Because of that I figured that these tests are more like unit tests with internal use of test_utils.

OK, fair enough. I was mainly wondering that they are kind of redundant or duplicates with the new reference test vectors. Let's leave them in then! more tests hardly ever hurt 🙂

@nothingismagick nothingismagick merged commit dc30b45 into dev Feb 24, 2021
@nothingismagick nothingismagick deleted the feat/ed25519-tests branch February 24, 2021 08:29
This pull request was closed.
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