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 spec for WebAuthn::PublicKeyCredential#verify #414

Conversation

soartec-lab
Copy link
Contributor

There was no test code for WebAuthn::PublicKeyCredential#verify, so I added it.

I noticed this when I updated lib/webauthn/public_key_credential.rb at #413, but the scopes were different, so I split the PR.

@santiagorodriguez96
Copy link
Contributor

Hey @soartec-lab!

We do have tests for the PublicKeyCredential but on its subclasses: PublicKeyCredentialWithAttestation (https://github.com/cedarcode/webauthn-ruby/blob/23f3be4/spec/webauthn/authenticator_attestation_response_spec.rb) and PublicKeyCredentialWithAssertion (https://github.com/cedarcode/webauthn-ruby/blob/6a5d7e9/spec/webauthn/public_key_credential_with_assertion_spec.rb). It could be said however, that the tests for PublicKeyCredentialWithAttestation, do not include tests like the one you are adding here. I think we can create a PR for that!

@soartec-lab
Copy link
Contributor Author

Hi @santiagorodriguez96 ,

Thank you for feedback.

Does that mean that the test should be provided in the subclass and there is no need to test for PublicKeyCredential?
Since PublicKeyCredential is a public class, I thought it would be necessary to test it. What do you think?

@santiagorodriguez96
Copy link
Contributor

Hi @santiagorodriguez96 ,

Thank you for feedback.

Does that mean that the test should be provided in the subclass and there is no need to test for PublicKeyCredential? Since PublicKeyCredential is a public class, I thought it would be necessary to test it. What do you think?

It's not that is not tested, is just that we are testing it indirectly. I think I'd lean towards having the tests for the PublicKeyCredential as shared examples and calling them from its subclasses. After all we never use the PublicKeyCredential class directly.

I also think that there are methods in PublicKeyCredential that we are not testing, so we should add those tests to the spec suite of its sub classes.

@soartec-lab
Copy link
Contributor Author

@santiagorodriguez96

Ok, i got it. you meam, since PublicKeyCredential is not used directly, there is no need to add any tests, and if i add tests, i add tests to the subclasses right?
If my understanding is correct, i will close this PR. thank you for tell me.

@santiagorodriguez96
Copy link
Contributor

santiagorodriguez96 commented Feb 14, 2024

@santiagorodriguez96

Ok, i got it. you meam, since PublicKeyCredential is not used directly, there is no need to add any tests, and if i add tests, i add tests to the subclasses right?

Yeah, pretty much! Although if we want to add tests for methods defined in PublicKeyCredential we could use shared examples for testing the methods in its subclasses 🙂

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