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

key: Improve unit tests #602

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tcharding
Copy link
Member

Improve the unit tests in the key module by doing:

  • Remove a bunch of "rand-std" (use hard coded keys instead)
  • Split big tests into smaller appropriately named tests
  • Move things around so we can better see whats being tested

Improve the unit tests in the `key` module by doing:

- Remove a bunch of "rand-std" (use hard coded keys instead)
- Split big tests into smaller appropriately named tests
- Move things around so we can better see whats being tested
@tcharding
Copy link
Member Author

I got this idea for a post you made someplace or other about not having tests when "rand-std" feature was disabled. If this PR is good we can probably mimic it for a bunch of other modules.

0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
0xff, 0xff, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00,
0x63, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63,
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also use hex_lit::hex!


let mut pk_bytes = [0u8; 33];
pk_bytes[0] = 0x02; // Use positive Y co-ordinate.
pk_bytes[1..].clone_from_slice(&PK_BYTES);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised clippy doesn't scream about copy.

PublicKey::from_slice(&[0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE + 1]),
Err(InvalidPublicKey)
SecretKey::from_slice(&[0; constants::SECRET_KEY_SIZE + 1]),
Err(InvalidSecretKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test isn't that great since all-zero keys are also invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice observation, I'll improve it.

assert!(kp.eq_fast_unstable(&kp2));
}

// FIXME(Tobin): I don't know what this is testing? There is no assertion or panic?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this tests that if an RNG generates an out-of-range key it is called again instead of panicking.

Copy link
Member Author

Choose a reason for hiding this comment

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

oooh, thanks man.

let (sk, pk) = s.generate_keypair(&mut rand::thread_rng());
assert_eq!(PublicKey::from_secret_key(&s, &sk), pk); // Sanity check.

// TODO: This would be better tested with a _lot_ of different tweaks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to this comment, maybe we should keep it random?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implying that running the tests multiple times improves coverage? The test space (a random Scalar) is so large that no amount of test re-runs in any way improves coverage so I think the use of random gives a false sense of coverage. I've not worked out how to test this (and the other tests still "rand-std") though, I tried playing with kani yesterday without success. Open to suggestions but I probably just need to play with it some more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. Maybe just test a few obvious things that are generally edge-cases - all zeroes, max...

@apoelstra
Copy link
Member

Maybe we should instead have a TestRng object that internally uses thread_rng when we have rand-std but otherwise uses a LCG or something (or even uses a custom super-biased RNG that likes te produce extreme values). Then our tests can just call this object.

@tcharding
Copy link
Member Author

or even uses a custom super-biased RNG that likes te produce extreme values

That's a cool idea.

@tcharding
Copy link
Member Author

Thanks for the reviews, I didn't respond to all your comments @Kixunil but I read them and I'll come back to this PR when I'm in a "psyched for testing" mood.

@tcharding
Copy link
Member Author

Seems "psyched for testing" is taking a while to materialise, converting to draft.

@tcharding tcharding marked this pull request as draft July 12, 2023 02:56
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