Skip to content

Conversation

@nbbeeken
Copy link
Contributor

Description

What is changing?

Fixes an error match assertion on node 20

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken force-pushed the NODE-5295-fix-test-node20 branch from 9b860d8 to 0123b5a Compare May 17, 2023 19:22
@nbbeeken
Copy link
Contributor Author

Passing socks5-csfle

@nbbeeken nbbeeken marked this pull request as ready for review May 17, 2023 19:37
@durran durran self-assigned this May 18, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 18, 2023
durran
durran previously approved these changes May 18, 2023
@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 18, 2023
dariakp
dariakp previously approved these changes May 18, 2023
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

can we make the PR description a bit better? reduce error message match strictness is pretty vague and won't be much help in our commit history

@nbbeeken nbbeeken changed the title test(NODE-5295): reduce error message match strictness test(NODE-5295): assert bypass mongocryptd connection fails with ECONNREFUSED May 18, 2023
@nbbeeken nbbeeken requested a review from baileympearson May 18, 2023 19:57
@nbbeeken nbbeeken force-pushed the NODE-5295-fix-test-node20 branch from e5b3b10 to 3a91450 Compare May 18, 2023 20:01
.to.have.property('name', 'MongoServerSelectionError');

expect(insertError).to.match(/connect ECONNREFUSED 127.0.0.1:27021/);
expect(insertError, 'Error must contain ECONNREFUSED').to.satisfy(
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than hard-coding logic for different Node versions, why not inspect the error and search for ECONNREFUSED?

Suggested change
expect(insertError, 'Error must contain ECONNREFUSED').to.satisfy(
expect(inspec(insertError), 'Error must contain ECONNREFUSED').to.match(/.*ECONNREFUSED.*/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would want to know if there was yet another shape that the error took, the information went missing from the message in node 20, we will return it to the message in some way in: https://jira.mongodb.org/browse/NODE-5296, added todos and updated the ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

From our public docs about our errors:

It is our recommendation to use instanceof checks on errors and to avoid relying on parsing error.message and error.name strings in your code. We guarantee instanceof checks will pass according to semver guidelines, but errors may be sub-classed or their messages may change at any time, even patch releases, as we see fit to increase the helpfulness of the errors.

We don't guarantee error messages. The error information is still present, the error is just structured differently. I don't think preserving the exact error info is important and so checking that the error itself is the correct error type is sufficient.

Copy link
Contributor Author

@nbbeeken nbbeeken May 19, 2023

Choose a reason for hiding this comment

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

Oh I was not thinking about public API / semver, just usefulness, as in an error with no message is not helpful so I don't want the test to not know about changes to the message if that's the regex we're looking for, in all cases, once we fix the node20 situation

@nbbeeken nbbeeken requested a review from baileympearson May 18, 2023 20:30
@nbbeeken nbbeeken force-pushed the NODE-5295-fix-test-node20 branch from 92d95bb to ab3605f Compare May 19, 2023 14:59
@durran durran merged commit 209a91d into main May 22, 2023
@durran durran deleted the NODE-5295-fix-test-node20 branch May 22, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team Review Needs review from team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants