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

refactor: testing matcher and logging #7363

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Oct 27, 2023

Refactors the error matcher to accommodate the fact that we know the shape of the things we are testing. This is only a change to our own matchers and is not publicly exposed. This was tested by intentionally breaking several tests and checking outcomes. The debugging information provided by the assertion is also improved.

I can't believe I didn't spend some time doing this ages ago. There's still room for improvement here, but this does make things easier to debug.

image
image
image

Refactors the error matcher to accommodate the fact that we know the shape of the things we are testing. This is only a change to our own matchers and is not publicly exposed. This was tested by intentionally breaking several tests and checking outcomes. The debugginginformation provided by the assertion is also improved
@benlesh benlesh closed this Oct 27, 2023
@benlesh
Copy link
Member Author

benlesh commented Oct 27, 2023

Rethinking this change. As most people will be using a deep equals algorithm that is less forgiving with notifications structures. (i.e. matching { kind: 'N', value: 1 } with { kind: 'N', value: 1, error: undefined }).

And maybe we should match that?

@benlesh benlesh reopened this Oct 27, 2023
@benlesh benlesh marked this pull request as draft October 27, 2023 20:33
@kwonoj
Copy link
Member

kwonoj commented Oct 27, 2023

fyi I made a custom matcher once:

image
image

https://github.com/kwonoj/rx-sandbox/blob/master/src/assert/marbleAssert.ts

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.

2 participants