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

Improve and Add Comprehensive Test Coverage [#71] #86

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Paul-Taiwo
Copy link
Contributor

This PR addresses the issue [#71]. Key changes include:

  1. New Test Suites:
  • Added client.test.ts to verify high-level registration/authentication flows via the WebAuthn API mocks.
  • Updates server.test.ts covering signature verification, challenge checking, and data parsing on the server side.
  • Update parsers.test.ts to ensure correct parsing of client data, authenticator data, and registration/authentication structures.
  1. Refined Mocks & Setup:
  • Centralized global mocks for navigator.credentials, PublicKeyCredential, crypto.subtle, etc. into a single Jest setup file, simplifying maintenance and reducing duplication.
  • Used environment checks (Node vs. jsdom) to conditionally load mocks, ensuring tests run smoothly in both environments.
  1. Enhanced Coverage:
  • Added tests for edge cases (e.g., missing challenges, invalid base64url).
  • Verified abort behavior for concurrent WebAuthn calls.
  • Tested fallback logic for missing AAGUIDs and updated metadata.
  • Test coverage up to 94.57%
image

@dagnelies
Copy link
Collaborator

That sounds awesome, can't wait to test it out 🚀

I'll review / try it later because I'm short on time right now, but at first glance I like it very much. Thanks 🙌

@dagnelies
Copy link
Collaborator

Hi @Paul-Taiwo. I reviewed your PR right now, and I noticed you moved a significant chunk of code from the server to utils. Why? Could you please revert that change? This would break lots of code out there. It would be ideal if this PR focuses solely on the test suite, not affecting the code itself.

tsconfig.json Outdated
@@ -15,7 +15,8 @@

},
"include": [
"./src/**/*.ts"
"./src/**/*.ts",
"global.d.ts"
Copy link
Collaborator

@dagnelies dagnelies Jan 27, 2025

Choose a reason for hiding this comment

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

I also think the "global.d.ts" should be moved to the test dir, or jest dir, and named "mocks" or so, to indicate clearly that it's a file for testing. Does it also have any impact on the production build?

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 points! The global.d.ts file is purely for typing the global mock variables. Regarding the production build, the declaration file don’t actually emit any JavaScript or impact runtime code, so they won’t alter the production bundle. Still, I've moved it into the jest_config file to keep things tidy and avoid confusion.

@Paul-Taiwo
Copy link
Contributor Author

Paul-Taiwo commented Jan 27, 2025

Hi @Paul-Taiwo. I reviewed your PR right now, and I noticed you moved a significant chunk of code from the server to utils. Why? Could you please revert that change? This would break lots of code out there. It would be ideal if this PR focuses solely on the test suite, not affecting the code itself.

The reason for splitting the functions out of the server.ts file is jest can’t mock same-file function calls by default. That means if verifyAuthentication() and verifySignature() (for example) both live in server.ts, any test that tries to mock verifySignature() will still end up calling the real function instead of the mock, because the file references it directly in-memory.

I’ll take another look to see if I can find an approach that won’t break the existing code.

@Paul-Taiwo
Copy link
Contributor Author

I have reverted the change regarding the chunk of code that was moved to utils from the server and I also updated/added more test. The coverage is now at 97.27%

image

@dagnelies
Copy link
Collaborator

Upon further inspection, there is really a lot that is mocked and real payloads not tested at all. In the end, it tests very little and the coverage is quite misleading.

@Paul-Taiwo
Copy link
Contributor Author

okay, I will review the test again.

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