chore(skill): Add skill for writing unit and E2E tests#20561
Conversation
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Great skill. You think it makes sense to highlight that we have reusable test utils that can be used?
E.g. in the e2e tests nothing is written WHAT to test (like errors, traces, metrics, etc)
|
|
||
| ### Arrange → Act → Assert | ||
|
|
||
| Structure every test with the AAA pattern, separated by blank lines. The whitespace makes the |
There was a problem hiding this comment.
l: I love this approach and used it a lot. However, does it make sense to separate the phases with whitespaces?
I only know it with strict comments:
# Arrange
// ... some arranging
# Act
// ... some acting
# Assert
// ... expect(false).toBe(true)There was a problem hiding this comment.
I don't think we need to be too explicit about that. AAA is more about the structure of the test, I don't have hard opinions on how the steps are separated.
|
|
||
| // Good: asserts on the observable outcome | ||
| it('sets transaction name from route path', () => { | ||
| responseHandler(createMockContext(200)); |
There was a problem hiding this comment.
l: Here there are no whitespaces. Is this now already following the AAA pattern? Not sure if LLMs might get confused already
| Default to exact matching. `toMatchObject`, `expect.objectContaining`, and `expect.arrayContaining` | ||
| silently ignore fields that matter. This has caused real bugs to ship in this codebase. | ||
|
|
||
| **Use `toEqual` unless you have a specific reason not to.** The same applies to |
There was a problem hiding this comment.
Use
toEqualunless you have a specific reason not to.
What would be a specific reason not to?
|
|
||
| ### Boundary Value Analysis | ||
|
|
||
| If the valid range is 1–100, test 0, 1, 2, 99, 100, 101. Bugs cluster at boundaries — off-by-one |
There was a problem hiding this comment.
| If the valid range is 1–100, test 0, 1, 2, 99, 100, 101. Bugs cluster at boundaries — off-by-one | |
| If the valid range is 1–100, test -10, 0, 1, 2, 99, 100, 101, Number.POSITIVE_INFINITY. Bugs cluster at boundaries — off-by-one |
|
|
||
| - Name test files `*.test.ts`, mirroring the source path: `src/shared/patchRoute.ts` → | ||
| `test/shared/patchRoute.test.ts`. | ||
| - Import from the source path (`../../src/...`), not from the package's published API. |
There was a problem hiding this comment.
l: Shouldn't we import from the published API if there is one? That would also then check for breaking changes in case we would by accident delete something somewhere
There was a problem hiding this comment.
For other packages, yes. But not for the package being tested. I'm gonna make that more clear.
| package, prefer the import form for type safety: | ||
|
|
||
| ```typescript | ||
| vi.mock(import('../../src/utils'), async importOriginal => { |
There was a problem hiding this comment.
l: I've seen a lot of times that this causes brutal type errors 😅 even though I would prefer it for the sake of type safety
There was a problem hiding this comment.
We can always fall back. But I added that because vitest recommends it: https://vitest.dev/guide/learn/mock-functions.html#mocking-modules
There was a problem hiding this comment.
Yes they recommend it and it makes sense, just wanted to highlight that in practice this is often a nightmare to use
|
|
||
| ### Grouping | ||
|
|
||
| Two levels of `describe` is usually enough. Deeper nesting makes tests harder to find and read. |
There was a problem hiding this comment.
l: It doesn't say if describe is actually needed. Would it be fine if we only have it() as top-level? With nx everything is grouped already so it would be too bad if it is not wrapped in at least one describe()
For that, it's worth splitting this skill into unit-test and E2E test instructions. I could do that in another PR. |
2 participants
Add skill for writing tests. Created and evaluated with https://github.com/anthropics/skills/blob/main/skills/skill-creator/SKILL.md
Used sources for "how to write good tests":