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

[ Chore ] Simplify tests #44

Merged
merged 3 commits into from
Jan 1, 2024
Merged

[ Chore ] Simplify tests #44

merged 3 commits into from
Jan 1, 2024

Conversation

mw10013
Copy link
Collaborator

@mw10013 mw10013 commented Dec 29, 2023

Closes #42 [ Chore ] Simplify Tests

@mw10013 mw10013 requested a review from dev-xo December 29, 2023 20:22
@dev-xo
Copy link
Owner

dev-xo commented Dec 30, 2023

Will review this as soon as I have 30 minutes, @mw10013! Thanks for taking the time to look again into this! The first tests were already good enough!

@dev-xo
Copy link
Owner

dev-xo commented Jan 1, 2024

This has been fantastic, @mw10013! Although the first approach was already really good, it had a bit of complexity added that could have made it slightly less readable than the current one.

Smart choices and really great abstractions, like setupFirstAuthPhase.

Not a big fan of invariant, and I usually prefer keeping the package management as minimal as possible and use the tools we have, but in this case, it fits really well.

Thank you so much for taking the time to look again into this!
Happy New Year, @mw10013!!

@dev-xo dev-xo merged commit 102f63a into dev-xo:main Jan 1, 2024
4 checks passed
dev-xo added a commit that referenced this pull request Jan 1, 2024
* add `TOTP_STRATEGY_OPTIONS`
* add `setUpFirstAuthPhase`
* chore: make eslint happy

---------

Author: @mw10013
Co-authored-by: Dev XO <devxo@mail.com>
@dev-xo dev-xo changed the title Simplify tests [ Chore ] Simplify tests Jan 6, 2024
@mw10013 mw10013 deleted the simplify-tests branch April 19, 2024 01:29
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.

[ Chore ] Simplify Tests
2 participants