[ Chore ] Update Tests

[mw10013]

This PR firms up the tests.

It checks more values within API mocks. Discovered cases where readTOTP was called with an undefined hash. Investigation of authenticate() revealed that session vars where getting typed as any with session.get(). ensureStringOrUndefined() narrows the type to string/undefined or throws error. Then typescript squiggled on places where the code assumed the session var was defined leading to narrowing type checks.

[dev-xo]

Also consistency, for example:

Here, we are calling expect like this:

await expect(() => 
  strategy.authenticate(request, sessionStorage, {
    ...AUTH_OPTIONS,
    successRedirect: '/',
  }),
).rejects.toThrow(ERRORS.INVALID_EMAIL)

While in the following test:

await strategy
  .authenticate(request, sessionStorage, {
    ...AUTH_OPTIONS,
    successRedirect: '/',
  })
  .catch((error) => error)

expect(createTOTP).toHaveBeenCalledTimes(1)

I could be wrong about this, but doesn’t the same way of testing accomplish the same result? It would be great to stick with one for better consistency and easier maintenance over time.

[dev-xo]

Tests pass and that's all that matters.

For me, the block-scoped tests inside tests, increased the difficulty of reading the code. A test should be relatively simple and easy to follow, leaving the harder or more complex ones for the End to End. I know you did what you had to do, and you executed it perfectly. I simply think it added a bit of visual complexity that wasn't that much before.

In any case, these are probably just my preferences, and as more people handle the codebase, fewer preferences will remain, and that's okay.

[mw10013]

Also consistency, for example:

Here, we are calling expect like this:

await expect(() => 
  strategy.authenticate(request, sessionStorage, {
    ...AUTH_OPTIONS,
    successRedirect: '/',
  }),
).rejects.toThrow(ERRORS.INVALID_EMAIL)

While in the following test:

await strategy
  .authenticate(request, sessionStorage, {
    ...AUTH_OPTIONS,
    successRedirect: '/',
  })
  .catch((error) => error)

expect(createTOTP).toHaveBeenCalledTimes(1)

I could be wrong about this, but doesn’t the same way of testing accomplish the same result? It would be great to stick with one for better consistency and easier maintenance over time.

@dev-xo: I think the second approach may be ambiguous. If authenticate() succeeds or throws an error, the outcome from the test perspective is the same since error is swallowed by the catch. I don't think we have any tests where we don't care if authenticate() succeeds or throws an error. I'll go through the tests again to clean this up.

I discovered that it's tricky to mock 1st Authentication Phase by simply generating a totp and then trying to patch up session vars. Our session vars are a little complex and need to have the totp hash in them. That wasn't the case with some test cases and then they would fail due to the fix in authenticate() that checks that a hash is defined in session vars.

In the end, it seemed better to let remix-auth-totp run the 1st Authentication Phase for the test in cases where a valid otp and hash in session were needed. But that makes the tests longer and more complicated. Perhaps we should have a test helper function that does all that for the tests that need it.

I apologize for not discussing what turned out to be a major reworking of the tests beforehand with you to get guidance and feedback. I will communicate and collaborate more going forward.

[dev-xo]

Perhaps we should have a test helper function that does all that for the tests that need it.

Sounds good to me @mw10013! I said this before, at this point, you understand the Strategy and the codebase better than me. I'm simply learning right now.

So feel free to take any approach you may think could be better. We just have to be consistent about it and keep TypeScript as happy as possible.

Nothing to apologize for; you've been doing more for the codebase than me before! And at this point where I'm a kinda busy, I feel so grateful to have some support from you on the library!