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

Use toStrictEqual in root tests #93

Merged
merged 4 commits into from
Oct 9, 2023
Merged

Use toStrictEqual in root tests #93

merged 4 commits into from
Oct 9, 2023

Conversation

72636c
Copy link
Member

@72636c 72636c commented Oct 6, 2023

toMatchObject accepts an object subset, which makes it hard to reliably test property removal. For example, #92 adds a happy-path test case "should omit defaultOmitHeaderNames by default" which unexpectedly passes even if the omission logic is removed.

These tests aren't great overall and I'd prefer them to use a more typical Jest structure so we can use .onlys and the like, but this should be enough to get #92 through with confidence around regression testing.

`toMatchObject` accepts an object subset, which makes it hard to
reliably test property removal. For example, #92 adds a happy-path test
case "should omit defaultOmitHeaderNames by default" which unexpectedly
passes even if the omission logic is removed.

These tests aren't great overall and I'd prefer them to use a more
typical Jest structure so we can use `.only`s and the like, but this
should be enough to get #92 through with confidence around regression
testing.
@72636c 72636c requested review from ConradLang and a team October 6, 2023 04:32
Copy link
Contributor

@ConradLang ConradLang left a comment

Choose a reason for hiding this comment

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

Agree with the use of toStrictEqual.

src/index.test.ts Outdated Show resolved Hide resolved
@72636c 72636c merged commit 8e647f1 into master Oct 9, 2023
2 checks passed
@72636c 72636c deleted the test-strict-equal branch October 9, 2023 03:24
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