Skip to content

test(NODE-5295): assert bypass mongocryptd connection fails with ECONNREFUSED #3673

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.

Sign up for GitHub

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
durran merged 4 commits into from
May 22, 2023
Merged

test(NODE-5295): assert bypass mongocryptd connection fails with ECONNREFUSED #3673

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0123b5a
test(NODE-5295): reduce error message match strictness
nbbeeken May 17, 2023
3a91450
fix: do not throw if cause does not exist
nbbeeken May 18, 2023
ab3605f
docs: add todos
nbbeeken May 18, 2023
9e49227
Merge branch 'main' into NODE-5295-fix-test-node20
durran May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions test/integration/client-side-encryption/client_side_encryption.prose.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,12 @@ describe('Client Side Encryption Prose Tests', metadata, function () {
.to.be.instanceOf(Error)
.to.have.property('name', 'MongoServerSelectionError');

expect(insertError).to.match(/connect ECONNREFUSED 127.0.0.1:27021/);
// TODO(NODE-5296): check error.message once AggregateErrors are handled correctly
expect(insertError, 'Error must contain ECONNREFUSED').to.satisfy(
Copy link
Contributor

@baileympearson baileympearson May 18, 2023

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

@nbbeeken nbbeeken May 18, 2023

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

@baileympearson baileympearson May 19, 2023

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 •
edited
Loading

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

error =>
/ECONNREFUSED/.test(error.message) ||
!!error.cause?.cause?.errors?.every(e => e.code === 'ECONNREFUSED')
);

expect(insertError).not.to.be.instanceOf(
MongoServerSelectionError,
Expand Down Expand Up @@ -1270,9 +1275,17 @@ TODO(NODE-5283): The error thrown in this test fails an instanceof check with Mo
client = new MongoClient('mongodb://localhost:27021/db?serverSelectionTimeoutMS=1000');
const error = await client.connect().catch(e => e);

expect(error)
// TODO(NODE-5296): check error.message once AggregateErrors are handled correctly
expect(
error,
'Error MUST be a MongoServerSelectionError error that contains ECONNREFUSED information'
)
.to.be.instanceOf(MongoServerSelectionError)
.to.match(/connect ECONNREFUSED 127.0.0.1:27021/);
.that.satisfies(
error =>
/ECONNREFUSED/.test(error.message) ||
!!error.cause?.cause?.errors?.every(e => e.code === 'ECONNREFUSED')
);
});
});

Expand Down