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

feat: refactor and add e2e tests #651

Merged
merged 20 commits into from
Sep 14, 2024
Merged

Conversation

DorianMazur
Copy link
Collaborator

@DorianMazur DorianMazur commented Aug 26, 2024

Motivation

This PR introduces end-to-end (e2e) tests to ensure the library functions as expected. The existing Java unit tests were outdated (last updated 3 years ago), only covered Android (with no iOS support), and were difficult to maintain. Due to these issues, I decided to remove the unit tests, as they were no longer helpful and were complicating the development process.

image

Description

This PR includes the following changes:

  1. Added e2e tests using Detox: I chose Detox over Maestro because Detox provides better support for biometrics and adb commands, even though both tools have a similar setup complexity (in my opinion)
  2. Refactored the codebase to TypeScript: I transitioned the code from Flux to TypeScript, as it is now a widely accepted standard.
  3. Reorganized code structure: I moved the Android-specific code to an android directory and the iOS-specific code to an ios directory for better organization.
  4. Updated the codebase: The code has been updated to support the latest versions of React Native.
  5. Integrated react-native-bob-builder: This tool is now used to build the library.
  6. Improved the example app: I refactored the example app to use react-native-test-app, making it easier to update to newer versions of React Native. This also sets the stage for potentially adding e2e tests for different React Native versions.

Copy link
Owner

@oblador oblador left a comment

Choose a reason for hiding this comment

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

Amazing! Left a few nits and a question around testing.

.github/workflows/lint_and_types.yaml Outdated Show resolved Hide resolved
KeychainExample/App.tsx Show resolved Hide resolved
KeychainExample/App.tsx Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like there are quite a few cases in this test suite not covered by the e2e from what I can tell. Can we cover them too (esp around which crypto engine picked and migration paths)?

Copy link
Collaborator Author

@DorianMazur DorianMazur Sep 13, 2024

Choose a reason for hiding this comment

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

Are you talking about RSA, AES and migration from FB to AES?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah

Copy link
Collaborator Author

@DorianMazur DorianMazur Sep 14, 2024

Choose a reason for hiding this comment

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

Done! 👍 Once the workflow is complete, I’ll merge it.

@DorianMazur DorianMazur merged commit 2066613 into master Sep 14, 2024
7 checks passed
@DorianMazur DorianMazur deleted the feat/refactor_and_add_e2e_tests branch September 14, 2024 15:38
@DorianMazur DorianMazur restored the feat/refactor_and_add_e2e_tests branch September 14, 2024 15:45
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