-
Notifications
You must be signed in to change notification settings - Fork 633
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
feat: add std/expect #3814
Conversation
Co-authored-by: Thomas Cruveilher <38007824+Sorikairox@users.noreply.github.com>
Very much looking forward to this. Curious though, would it make sense to put this under |
We thought the |
I'm in favor of keeping the implementation in |
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
|
We generally prefer top level modules over nested structure. For example, we moved some complex modules from |
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 |
9f6ab16
to
b1d19a4
Compare
There was a problem hiding this 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
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); | ||
} | ||
} |
There was a problem hiding this comment.
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?
expect/_to_be_close_to.ts
Outdated
import { MatcherContext, MatchResult } from "./_types.ts"; | ||
import { AssertionError } from "../assert/assertion_error.ts"; | ||
|
||
// TODO(kt3k): tolerance handling is wrong |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Fair enough. I'll move all matchers into |
Landing this as |
Continued from #3627
This PR adds
std/expect
mostly compatible withexpect
of jest.Remaining:
Mock related:
Testing:
We'd avoid supporting snapshot related APIs for first pass (
toMatchSnapshot
toMatchInlineSnapshot
toThrowErrorMatchingSnapshot
toThrowErrorMatchingInlineSnapshot
)Done:
closes #3813 1779