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 security module against actual indentity. #30

Merged
merged 5 commits into from
Mar 18, 2018

Conversation

A4Vision
Copy link
Contributor

Simplify the tests, compare client usage against direct usage of Identity.

@kamyuentse kamyuentse requested a review from realcr March 11, 2018 13:46
@@ -76,25 +76,15 @@ mod tests {
}
})).unwrap().public_key;

let rsender = requests_sender.clone();
Copy link
Member

@realcr realcr Mar 13, 2018

Choose a reason for hiding this comment

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

@A4Vision : The purpose of this test was to make sure that RequestPublicKey returns a consistent result from the security module (Even when called twice). I propose one of the following:

  1. Add the comparison of actual_public_key against public_key1
  2. Leave this test as it is, and add a new test that compares actual_public_key and public_key_from_client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye ! I see - I did weaken the test, so I fix it to solution 1.
In this case, a "for" loop is simpler than previous code - so I add a for loop.

@realcr
Copy link
Member

realcr commented Mar 13, 2018

@A4Vision : The tests in this module were meant to check the interface of SecurityModule. Indeed we can make the tests shorter if we use the internal identity directly, but then we are relying on the underlying interface of identity.

I think that the tests should only deal with the interface of SecurityModule. If we call methods of identity directly we mix the two interfaces. What is your opinion?

@A4Vision
Copy link
Contributor Author

A4Vision commented Mar 17, 2018

The tests in this module were meant to check the interface of SecurityModule ... we are relying on the underlying interface of identity.

@realcr : Generally I agree that separating testing of interfaces is better when reasonable.
In this specific case, the length is more significant.

In the second test (test_security_module_request_sign), I think the modified test is better because it separates the test of getting the public key from the test of getting a signature.

@realcr
Copy link
Member

realcr commented Mar 17, 2018

@A4Vision : I restarted the travis job, because it seemed to fail on the famous timer test. Hopefully it will pass this time.

Regarding the second test: fn test_security_module_request_sign, I think that each of us wants to test something a bit different. The way I see it, I think that it is very important to test the interface of SecurityModule, and for you it is important to have a short test.

I propose that you add another test with the code the you like (Directly calling let public_key = identity.get_public_key();). Having an extra test always upgrades the party.

@A4Vision
Copy link
Contributor Author

A4Vision commented Mar 17, 2018

I propose that you add another test

Done.

Discussing it any further simply does not worth our time.

….com/realcr/cswitch into assaf/fix/security-module-correct-test
@realcr
Copy link
Member

realcr commented Mar 17, 2018

Upstream problem with rust compiler + clippy, reported here:
rust-lang/rust#49114

@kamyuentse
Copy link
Collaborator

kamyuentse commented Mar 18, 2018 via email

@realcr realcr merged commit 4aa683b into master Mar 18, 2018
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