Skip to content

test: bson corpus spec runner calling wrong fn #436

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

Merged
merged 2 commits into from
May 25, 2021
Merged

Conversation

durran
Copy link
Member

@durran durran commented May 12, 2021

I ran across this one-liner when looking into Decimal128 usages. (Of course they all were passing since Decimal.128.fromString would never be able to deserialize the EJSON, so we now use the EJSON parse helper function, which passes, but aren't false positives)

NODE-3277

@durran durran requested review from a team, emadum, nbbeeken and dariakp and removed request for a team May 12, 2021 18:39
@durran durran force-pushed the fix-corpus-spec-runner branch from ede54d2 to 69150d6 Compare May 12, 2021 18:47
@dariakp
Copy link
Contributor

dariakp commented May 12, 2021

@durran Looks good but I think per our convention, the commit message should be test: ... not fix: ... since we aren't fixing any functionality in the library itself

@durran durran changed the title fix: bson corpus spec runner calling wrong fn test: bson corpus spec runner calling wrong fn May 14, 2021
@durran durran force-pushed the fix-corpus-spec-runner branch from 69150d6 to 8b85a70 Compare May 14, 2021 14:53
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

The require for Decimal128 at the top also needs to be removed, currently it's causing a lint error in CI

@emadum emadum self-requested a review May 25, 2021 12:03
@durran durran merged commit 485aa3c into master May 25, 2021
@durran durran deleted the fix-corpus-spec-runner branch May 25, 2021 13:19
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.

4 participants