fix(dns): remove lookupPromise polyfill for node8 dns promise tests #1223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `lookupPromise` function emulates a Promise based dns.lookup by creating its own Promise() object, and then calling the async callback version of `dns.lookup()` inside it. While this does make the tests work under Node 8, it is not at all what these tests were originally intended to test. That is, all of the functions in this file were testing the polyfill in this test file, and *not* testing the promise based version of dns.lookup() that was introduced in Node 10.6.0. Tellingly, the tests in this code never exercise the patch code in instrumentation.ts that handles the non-callback (i.e. Promise-based) version of the lookup() function. This change removes the irrelevant test function and instead reverts to directly calling dns.promises.lookup() as existed in this code prior to the "cleanup" in PR# 110 that nominally was "Setting up ESLint".
Flarna
approved these changes
Oct 10, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
In #110 the
dnspromise based tests were reinstated for node 8 via a "polyfill"-style function in the test source withthis comment.
It's unclear to me what exactly was being attempted here, since the net result of making all calls in the test function funnel through this
lookupPromise()function was that the functionality that was supposed to be tested (i.e.dns.promises.lookup()was no longer tested at all.This may have been missed because code coverage was probably not run as part of CI then, but looking at code coverage now, the code in
instrumentation.tsthat handles patching promise based calls is uncovered by the tests in this file.Short description of the changes
lookupPromise()test polyfill fordns.promise.lookup()dns.promises.lookup()function.