firefox-devtools / firefox-devtools/profiler
Document our testing practices
- Dominant language
- TypeScript
- Stars
- 1.5k
- Forks
- 491
- Avg merge
- 3d 46m
- Merged PRs (30d)
- 27
Description
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](https://mozilla-hub.atlassian.net/browse/FP-712)
Contributor guide
Assessment
This issue has not been assessed yet.