Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add std/expect #3814

Merged
merged 17 commits into from Nov 21, 2023
Merged

feat: add std/expect #3814

merged 17 commits into from Nov 21, 2023

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Nov 16, 2023

Continued from #3627

This PR adds std/expect mostly compatible with expect of jest.

Remaining:

  • toHaveLength
  • toHaveProperty
  • toContain
  • toContainEqual
  • Fix toBeCloseTo tolerance handling

Mock related:

  • fn (mock)
  • toHaveBeenCalled
  • toHaveBeenCalledTimes
  • toHaveBeenCalledWith
  • toHaveBeenLastCalledWith
  • toHaveBeenNthCalledWith
  • toHaveReturned
  • toHaveReturnedTimes
  • toHaveReturnedWith
  • toHaveLastReturnedWith
  • toHaveNthReturnedWith

Testing:

  • add test cases

We'd avoid supporting snapshot related APIs for first pass (toMatchSnapshot toMatchInlineSnapshot toThrowErrorMatchingSnapshot toThrowErrorMatchingInlineSnapshot)

Done:

  • documentation

closes #3813 1779

kt3k and others added 2 commits November 16, 2023 11:44
Co-authored-by: Thomas Cruveilher <38007824+Sorikairox@users.noreply.github.com>
@andrewthauer
Copy link

Very much looking forward to this. Curious though, would it make sense to put this under std/testing or may std/testing/expect instead?

@iuioiua
Copy link
Collaborator

iuioiua commented Nov 16, 2023

We thought the expect() implementation, once complete with testing, would be of a sufficient size that it might justify having it as its own sub-module. Plus, it'd be more visible.

@kt3k
Copy link
Member Author

kt3k commented Nov 16, 2023

Curious though, would it make sense to put this under std/testing or may std/testing/expect instead?

I'm in favor of keeping the implementation in std/expect, but I'm open to the idea of re-exporting this from somewhere in std/testing, such as from std/testing/bdd.ts.

@andrewthauer
Copy link

Out of curiosity, is the plan to keep all modules at the top level or will there be any nesting?

I personally think I'd find it more intuitive to scan top level areas first, then drill into them further. For example I'd find testing and then look inside to see bdd and expect, etc.. A possible structure might look like:

└── testing
    ├── bdd
    │   └── mod.ts
    ├── expect
    │   └── mod.ts
    └── mod.ts

@kt3k
Copy link
Member Author

kt3k commented Nov 16, 2023

We generally prefer top level modules over nested structure. For example, we moved some complex modules from encoding/ to top-level to avoid the nested structure in the past. cf #3123

@andrewthauer
Copy link

andrewthauer commented Nov 16, 2023

Right. For encoding that makes sense to me. I personally wouldn't think to look under that initially for json, yaml, etc.

I would say that expect is much less clear to me own its own at the top level then the encoding ones. Putting it under testing would give it context and make it more clear. My 2 cents anyways. I'll will say no more now 😀.

@kt3k kt3k self-assigned this Nov 17, 2023
@kt3k kt3k marked this pull request as ready for review November 21, 2023 09:37
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks mostly good to me, I'm a bit worried about a number of modules that were added - if you're unit testing a file that has no dependencies besides pulling expect/mod.ts then the overhead of loading all these small modules might be non-trivial. Could you reconsider adding a single module that has all the matchers inside it?

expect/_to_be.ts Outdated
Comment on lines 3 to 13
import { MatcherContext, MatchResult } from "./_types.ts";
import { assertNotStrictEquals } from "../assert/assert_not_strict_equals.ts";
import { assertStrictEquals } from "../assert/assert_strict_equals.ts";

export function toBe(context: MatcherContext, expect: unknown): MatchResult {
if (context.isNot) {
assertNotStrictEquals(context.value, expect, context.customMessage);
} else {
assertStrictEquals(context.value, expect, context.customMessage);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for splitting into so many small modules?

import { MatcherContext, MatchResult } from "./_types.ts";
import { AssertionError } from "../assert/assertion_error.ts";

// TODO(kt3k): tolerance handling is wrong
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that covers this tolerance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this has been already resolved in 357ecb3

@kt3k
Copy link
Member Author

kt3k commented Nov 21, 2023

if you're unit testing a file that has no dependencies besides pulling expect/mod.ts then the overhead of loading all these small modules might be non-trivial.

Fair enough. I'll move all matchers into _matchers.ts

@kt3k
Copy link
Member Author

kt3k commented Nov 21, 2023

Landing this as std/expect for now. Let's revisit std/expect vs std/testing/expect before stabilizing this.

@kt3k kt3k merged commit 1a7ede0 into denoland:main Nov 21, 2023
10 checks passed
@kt3k kt3k deleted the std-expect-prepare-2 branch November 21, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants