MNTOR-1101 run V2 unit tests using Github Actions

[rhelmer]

Run V2 unit tests using GitHub Actions.

[rhelmer]

Here is my understanding of the Jest+ES situation, based on reviewing the docs and issue below:

Jest has some issues with ES modules, so this PR uses Babel to transpile. The alternative is to make some ugly modifications to the test code and enable experimental support in node:

• https://jestjs.io/docs/ecmascript-modules
• https://github.com/facebook/jest/issues/9430

I feel like using babel for now and then just removing it as node and jest stabilize would be easiest, but open to trying to go pure ES now if we want to. Note that it's not just enabling the experimental ES support in node, you also need to change the jest calls in the unit test code (per the docs above), e.g. jest.unstable_mockModule, and hoisting doesn't work yet with ES so you need to make that call first then do a dynamic import (const { myThing } = await import('myModule') - that's what I mean specifically by modifications to the test code.

Using Babel allows us to write the tests as ES and it gets transpiled to CJS which Jest understands, so you can write unit tests the normal way:

import { myThing } from 'myModule'
jest.mock('myModule') // myThing is now automatically mocked

[toufali]

Thanks for explaining the problem, and agreed that the workaround using jest.unstable_mockModule seems awful! I'm mostly on board with the Babel workaround, but a couple thoughts:

• Should we be concerned with Jest testing babel-transformed code, but in production the code is not transformed? Wouldn't it be better to test the actual (un-transformed) code?
• This blurb seems to suggest there's a block for Jest mocks to ever work nicely with ESM. It makes me wonder if Jest mocks will ever work without dynamic imports... If so, simply removing Babel in the future without code change may not be realistic?
• Is the ability to use mocks the main reason for this work-around? If so, maybe we can just avoid their use? I've read mocks can sometimes suggest code-smell!
• With all the above, should we consider a solution like esmock? It could be used with Jest, or maybe better (as the examples show) on top of the native Node 18+ test runner?

[rhelmer]

Thanks for explaining the problem, and agreed that the workaround using jest.unstable_mockModule seems awful! I'm mostly on board with the Babel workaround, but a couple thoughts:

* Should we be concerned with Jest testing babel-transformed code, but in production the code is not transformed?  Wouldn't it be better to test the actual (un-transformed) code?

Ideally yes.

* [This blurb](https://jestjs.io/docs/ecmascript-modules#module-mocking-in-esm) seems to suggest there's a block for Jest mocks to ever work nicely with ESM.  It makes me wonder if Jest mocks will ever work without dynamic imports...  If so, simply removing Babel in the future _without_ code change may not be realistic?

Hm, I think you're right, this might just be a consequence of moving to ES that imports are always hoisted to the top... we may need to use dynamic imports in the future no matter what, so it might be worth doing that now.

* Is the ability to use mocks the main reason for this work-around?  If so, maybe we can just avoid their use?  I've read mocks can sometimes suggest code-smell!

Do you have any references I can read? I'm not sure what the alternatives to mock would be for certain cases, wouldn't that imply that we need to modify the code to be testeable? I think the advantage of mocks is that the code under test doesn't need special modifications to be able to override behavior to be more appropriate in the test environment.

* With all the above, should we consider a solution like [esmock](https://www.npmjs.com/package/esmock)?  It could be used with Jest, or maybe better (as the examples show) on top of the native Node 18+ test runner?

This looks promising, thanks! I'll see if I can get this running. I feel like moving away from mocks completely would be a pretty large change, esmock looks like it might allow us to reuse more of the existing test code. Most of the test code I've seen so far seems pretty sensible (e.g. tests that nodemailer receives the expected inputs and overrides the ability to send outgoing email with a mock that can be inspected etc.)

[toufali]

Do you have any references I can read?

Yep, this is what I got into yesterday:
https://dev.to/kettanaito/when-should-i-not-use-mocks-in-testing-544e
https://chemaclass.medium.com/to-mock-or-not-to-mock-af995072b22e
https://medium.com/javascript-scene/mocking-is-a-code-smell-944a70c90a6a

Interesting to note the slight contradictions in the articles with regard to unit test mocks. I think the big take-away will always be "it depends".

[rhelmer]

Do you have any references I can read?

Yep, this is what I got into yesterday: https://dev.to/kettanaito/when-should-i-not-use-mocks-in-testing-544e https://chemaclass.medium.com/to-mock-or-not-to-mock-af995072b22e https://medium.com/javascript-scene/mocking-is-a-code-smell-944a70c90a6a

Interesting to note the slight contradictions in the articles with regard to unit test mocks. I think the big take-away will always be "it depends".

Thanks. I think it's worth digging into this more, right now we have a bunch of existing test code, although our coverage is quite low - I think it's worth continuing with mocking but we should consider whether there are better approaches.

I think in particular we'd benefit from end-to-end testing using Playwright or similar, which i know @mozrokafor already did some work on. I don't think the existing unit tests make excessive use of mocking, at least not the few that I have ported over so far (email-utils and the settings controller).

[rhelmer]

@toufali OK here's a different approach, using the minimal test runner Ava which is pure ES.

The API isn't quite mocha compatible but it's similar. I've ported over the tests that @mansaj added in ./src/*.test.js, neither of these use mocks, so I don't think we need to worry about introducing esmock or similar quite yet.

[toufali]

Oh wow, this update looks fantastic and runs super fast! Awesome work – I appreciate the extra diligence here 🙌

[mansaj]

There were some more tests added into /utils yesterday just FYI

[rhelmer]

There were some more tests added into /utils yesterday just FYI

Thanks, I'll rebase and convert those over as well.