Document our testing practices

[gregtatum]

I think it would be worthwhile revisiting and discussing some of our testing practices and getting a consensus on the approach.

In #833 there were concerns raised about any type of setup logic being done in the describe block, even if it is immutable data. This hasn't particularly bothered me, especially with some of the guarantees of our immutable practice. However, the risk isn't 0%, in case we are subtly mutating things and are not realizing it.

One thing I really don't like is the side effect nature of beforeEach and afterEach. I think resetting variables through side-effects to be really difficult to understand what is going on. Sometimes I have introduced additional complexity due to avoiding using beforeEach and afterEach in order to contain or remove side-effects. I did this a lot with my mocks.

In order to work well with these different approaches, I propose the following practice moving forward:

• Disallow setup logic in the describe blocks.
• Setup should be contained in either:
  • Inside the it block.
  • A setup function defined in the describe block. This setup function should then be used to destructure out the variables that are needed.
• beforeEach and afterEach can be used freely, but avoid using them to reset local variables through side effects. The side effects make the it blocks hard to understand. Prefer the setup function. One good example usage of beforeEach and afterEach is to setup something like the window.geckoProfilerPromise which does not fit well into jest's mocking system (unless I'm missing something)
• Do not use the withAnalyticsMock approach. Prefer jest's automatic releasing of mocks.
• Use jest's mocking rather than sinon.

┆Issue is synchronized with this Jira Task

[gregtatum]

However, the risk isn't 0%, in case we are subtly mutating things and are not realizing it.

Literally I just modified a test and it failed because the snapshot was different due to a stringTable being mutated.

[julienw]

In #833 there were concerns raised about any type of setup logic being done in the describe block, even if it is immutable data.

To be clear, what I warned about was because of the use of the store. The store, by nature, isn't immutable, and if we have several tests in one describe block sharing the same store, we'll have interdependent tests.

For example I have nothing against defining the profile in the describe block, when we use it in a immutable way (or at least in an idempotent way: our upgraders mutate the profile, but when we don't test the upgraders themselves it's OK to reuse it).

One thing I really don't like is the side effect nature of beforeEach and afterEach.

I'm not sure I understand what you mean by "side effects". Is it when we setup local variables to be used in the tests?

IMO the biggest drawback of beforeEach and afterEach is that they're too far away from the test itself. When they're too much loaded it's difficult to understand the test as it requires keeping in memory what beforeEach/afterEach do too, and we usually don't have them in the current window.

I think, like you, that having a setup method that returns just the data we need in the test is much easier to understand.

A setup function defined in the describe block. This setup function should then be used to destructure out the variables that are needed.

And some of the most useful setup functions can also be in other files, eg storeWithProfile.

One good example usage of beforeEach and afterEach is to setup something like the window.geckoProfilerPromise which does not fit well into jest's mocking system (unless I'm missing something)

Yep, as far as I know, jest's spyOn only works on existing functions, so you can't use it and rely on its auto-restore mechanism to add new properties.

Another question is: beforeEach or beforeAll. I tend to prefer beforeEach to force independent tests.

Do not use the withAnalyticsMock approach. Prefer jest's automatic releasing of mocks.

In the case of withAnalyticsMock, we're adding a non-preexistent properties, so we can't use jest's mocking capability.

Actually, I quite like this approach over the use of beforeEach for geckoProfilerPromise (for the same reason that you outlined above), although it makes the test more verbose as we need to repeat this over and over + it adds some indentation. The main question is: does having the setup in beforeEach make the test more or less readable/maintainable.

In the case of withAnalyticsMock:

• we need to repeat it everywhere, while adding it to beforeEach is done only in one plce
• we actually test self.ga in the test

Use jest's mocking rather than sinon.

We use sinon in src/test/store/receive-profile.test.js only at the moment. I used it over Jest because:

• it allowed to easily stub setTimeout to ignore its timeout parameter. But actually Jest makes it even easier and more synchronous with its methods jest.runAllTimers() and jest.runOnlyPendingTimers().
• Sinon also made it easier to return a resolved value (or at least less verbose). But with our current version of Jest we also have mockResolvedValue.

I think we could completely remove sinon from our test suite.

[julienw]

There's pretty much a consensus on how to write tests and we should document basically what Greg wrote. The part about Sinon is moved to #1977.