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

Add test for PartialEq impl #58

Closed
wants to merge 1 commit into from
Closed

Add test for PartialEq impl #58

wants to merge 1 commit into from

Conversation

jesseli2002
Copy link

Part of #1. Adds test for equality operator overload.

Apologies if I'm doing something dumb - I'm coming from a C++ background and figured I should learn some Rust, and may as well try to contribute to the community while I learn.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@briansmith
Copy link
Owner

Apologies if I'm doing something dumb - I'm coming from a C++ background and figured I should learn some Rust, and may as well try to contribute to the community while I learn.

Thanks for contributing. Your PR was helpful, though indirectly and differently than what you intended. It prompted me to remove the PartialEq and Eq implementations from Input to make Input more suitable for processing secret inputs. See PR #60 and #65.

Accordingly, this PR now doesn't make sense, so I'll close it. I will still review your PR line-by-line anyway in case you find the input helpful.

@briansmith briansmith closed this Jul 13, 2021
@@ -62,6 +62,24 @@ fn test_input_as_slice_less_safe() {
assert_eq!(input.as_slice_less_safe(), slice);
}

#[test]
fn test_input_equality() {
let slice1 = b"foo";
Copy link
Owner

Choose a reason for hiding this comment

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

In general I prefer to have tests that are "parameterized"; that is, where there is an explicit list of test inputs (and maybe expected result for each one, if the expected result isn't the same for all of them), where the test loops over the list of inputs. This way it is clearer to see what is being tested for each input. See recent PRs for an example of this style.

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.

2 participants