Rewrite tests

[balazsorban44]

Describe the problem you'd like to have solved

I propose a change to how tests are written after the discussion here: #40 (comment)

Describe the ideal solution

As mentioned, I would use the concept Avoid Nesting when you're Testing, in addition to avoid writing repetitive configs by gathering them on top of each file (or another centralized place like mockData.js or something similar), and create a setup***() function for tests in a single test suite (file).

Reducing the number of nested describe() blocks would also improve readability, by flattening out the test suite. This would be much easier to achieve with the above mentioned setup***() functions.

These are not BREAKING CHANGES 💥, as it only touches on the testing part. Though, it must be done carefully, so everything that is tested already is migrated first, and then maybe try to think about adding more tests.

Additional context

I have mainly used Jest 🃏 for testing, and after working on the logout.test.js tests, I would much prefer to do that here as well. Mocha ☕ felt a bit cumbersome, and the outputs were not always easily readable IN MY OPINION.

• If I get green light at work, I may have a bit more time on my hands to work on this then doing it in my free-time. ✔ Green light

[joshcanhelp]

Thanks for the issue ahead of changes!

I'm totally open to how this works but it would be great to see a specific example in code, either as a draft PR or here in this issue. I know you had an example in that previous PR but we've since reverted that.

Also keep in mind that this library is in motion and will see a number of changes in the next week or two, including within the tests. Big re-writes across multiple files will go out of date quickly and likely prove hard to rebase. Best bet is to choose a single, small testing file that requires these changes and submit a PR for that first. If we go file-by-file, that will make this far less painful :)

Finally ... please be cautious about changes in coverage, positive or negative. Not to say that our tests are perfect, I'm sure we'll find some gaps, but changes to the assertions themselves should always be a red flag.

RE: Jest ... I'm not married to Mocha at all. If that's important to change, that should probably happen first, though then we're getting into huge PR territory ...

[balazsorban44]

Also keep in mind that this library is in motion and will see a number of changes in the next week or two, including within the tests. Big re-writes across multiple files will go out of date quickly and likely prove hard to rebase. Best bet is to choose a single, small testing file that requires these changes and submit a PR for that first. If we go file-by-file, that will make this far less painful :)

Sounds like a reasonable thing to do, given there might be big changes in the near future. I will create a small draft PR with logout tests again to begin with.

Finally ... please be cautious about changes in coverage, positive or negative. Not to say that our tests are perfect, I'm sure we'll find some gaps, but changes to the assertions themselves should always be a red flag.

My point was trying to be, I would try to keep all the assertions that are in place now, and only add new ones after the old ones are migrated.

RE: Jest ... I'm not married to Mocha at all. If that's important to change, that should probably happen first, though then we're getting into huge PR territory ...

I am going to try to write 2 test files, one for Mocha and one for Jest. Then we can compare the time it takes to run tests, and the DX (eg.: readable/helpful error messages on failing tests, watch mode, etc.) It may very well be the case, that I simply did not use Mocha enough. I have no intention to make unnecessary changes. 🙂

[balazsorban44]

Hi @joshcanhelp! 👋 Circulating back on this, as I have now some free time. I created a draft PR, you can have a look at it. If it would still be interesting, please give a 👍, and I shall continue the migration. If you have moved on from the idea, it is also fine. 🙂

[adamjmcgrath]

Have closed the PR and will close this, we're tracking some updates to the tests internally at the moment. Thanks again for your contributions