[ 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]

Is there any way we could avoid using "!" in our code, @mw10013? I have a rule against it, and my editor is complaining badly.

I'm rather strict with TypeScript and I like to follow the safe and conventional paths as much as possible.

---

Here's a few solutions provided by Chat GPT:

The TypeScript error you're encountering, Forbidden non-null assertion.eslint@typescript-eslint/no-non-null-assertion, is related to the usage of the non-null assertion operator !. This operator is used to tell TypeScript that you are certain a value is not null or undefined.

In your code, you have session!. This tells TypeScript to assume session is not null or undefined. However, the TypeScript linter rule @typescript-eslint/no-non-null-assertion is set up to prevent this because non-null assertions can be unsafe. They bypass TypeScript's strict null checks, potentially leading to runtime errors if session is actually null or undefined.

To resolve this, you have a few options:

1. Check for null/undefined: Before using session, check if it's not null or undefined.

   if (session) {
       cookie: await sessionStorage.commitSession(session),
   }

2. Use Optional Chaining: If sessionStorage.commitSession can handle undefined, use optional chaining.

   cookie: await sessionStorage.commitSession(session ?? undefined),

3. Provide a Default Value: If there's a sensible default value for session, use it.

   cookie: await sessionStorage.commitSession(session ?? defaultSession),

The best approach depends on your specific use case and how session is used in the rest of your code. Generally, it's best to handle potential null or undefined values explicitly to avoid runtime errors.

[dev-xo]

You did a great job as always, @mw10013, but the tests has become somewhat a bit harder to maintain than it was before (for me).

It would be amazing if we could discuss any new approach we will take in the code before implementing it. For example, these detailed tests, or the way to implement them, would be great to discuss somehow beforehand, that way I can also keep track of what's going on with the implementation.

All good though, you seem to handle this better than me, I simply want to try to catch up with things and not brainstorm that much about huge changes.

[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!