denoland / denoland/std

Proposal: Pass `expect` as a param in bdd tests

Open
#6,541 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
3.6k
Forks
681
PR merge metrics
No merged PRs in 30d

Description

**Is your feature request related to a problem? Please describe.**

Leads to natural solutions to
- #6518: `expect` outside `@std/testing` tests will not have the methods that are failing
- #6540: Replacing the global `AssertionState` with one passed in the context guarantees assertions are counted in the right context, and possibly in parent contexts

**Describe the solution you'd like**

- Create a new `interface ItTestContext extends Deno.TestContext` which can include an `AssertionState`
- Make `it` and `test` use `ItTestContext` instead of `Deno.TestContext`
- Remove `@std/expect`'s `expect.hasAssertion()` and `expect.assertions()` to prevent them from being used outside `bdd` tests
- Add a way to fetch an instance of `expect` from a `bdd` test
- This instance does support `expect.hasAssertion()` and `expect.assertions()`

Options for syntax:

```ts
import { makeExpect } from "@std/testing/expect";
test("test1", (t) => {
const expect = makeExpect(t);
expect.assertions(0);
});
```

```ts
test("test1", (t) => {
t.expect.assertions(0);
// This could have some issues where a user uses the wrong expect by accident,
// for example this assertion would not be counted:
// import { expect } from "@std/expect";
// expect(1).toBe(1);
});
test("test2", (t) => {
const { expect } = t;
expect.assertions(0);
});
```

```ts
test("test1", (_t, expect) => {
expect.assertions(0);
});
```

```ts
test("test1", (t) => {
// No expect at all, just put the assertions tests into the context
t.assertions(0);
});
```

**Describe alternatives you've considered**

~~I don't see a way to improve #6518 without moving `AssertionState` to some sort of context: The alternative is to add some sort of way for `expect` to know whether it's being called in a `bdd` test. If not, it would need to avoid counting assertions, and throw in the assertions test functions.~~

There might be a way for #6518 to be fixed by having it track whether it's been cleaned up and throw if not. The cleanup would happen in bdd tests but not in raw `Deno.test`s.

For #6540, the direct issue can be solved by only throwing the error after cleanup. However, I think it's still unintuitive that the same `AssertionState` is used for multiple tests. Even Jest has this issue:

```ts
describe('a', () => {
// Fails with "expected 2, found 1", which does not make sense:
// If we were counting only top-level, this should find 0
// If we are counting in all children, this should find 2
expect.assertions(2);

test("1", () => {
expect(1).toEqual(1);
});

test("2", () => {
expect(1).toEqual(1);
});
});
```

More extreme alternatives could involve preventing `expect` from running outside `bdd` tests. This would prevent users from using different sources of `expect`, but would also cause `expect` to be unavailable outside `Deno.test()` which is probably not a good idea.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.