test-dns-promises-exists.js cannot detect setDefaultResultOrder is missing from dns/promises

[F3n67u]

$ git diff test/parallel/test-dns-promises-exists.js 
diff --git a/test/parallel/test-dns-promises-exists.js b/test/parallel/test-dns-promises-exists.js
index 5fd20dff2c..814ea2c69e 100644
--- a/test/parallel/test-dns-promises-exists.js
+++ b/test/parallel/test-dns-promises-exists.js
@@ -2,5 +2,6 @@
 
 require('../common');
 const assert = require('assert');
-
+console.log('dnsPromises.setDefaultResultOrder is: ', require('dns/promises').setDefaultResultOrder)
+console.log('dns.promises.setDefaultResultOrder is: ', require('dns').promises.setDefaultResultOrder)
 assert.strictEqual(require('dns/promises'), require('dns').promises);

$ ./out/Release/node test/parallel/test-dns-promises-exists.js                      
dnsPromises.setDefaultResultOrder is:  undefined
dns.promises.setDefaultResultOrder is:  [Function: setDefaultResultOrder]

setDefaultResultOrder function is not defined in dns/promises, but does defined in require('dns').promises, but test/parallel/test-dns-promises-exists.js cannot detect this problem.

This make test/parallel/test-dns-promises-exists.js cannot check if require('dns/promises') and require('dns').promises have the same effect: this test cannot check setDefaultResultOrder is missing from require('dns/promises') but does exist in require('dns').promises.

Could anybody guide me on how to improve the test to make require('dns/promises') and require('dns').promises have the same effect?

test/parallel/test-fs-promises-exists.js have the same problem.

[aduh95]

When you write fs/promises, do you mean dns/promises? I'm going to assume that yes, but you wrote it four times in a row, so not sure.

There's a bug in the code where properties are added to node:dns/promises only if someone tries to access require('node:dns').promises:

node/lib/dns.js

Lines 382 to 391 in b5ed1bd

	ObjectDefineProperties(module.exports, {
	promises: {
	configurable: true,
	enumerable: true,
	get() {
	if (promises === null) {
	promises = require('internal/dns/promises');
	promises.setServers = defaultResolverSetServers;
	promises.setDefaultResultOrder = setDefaultResultOrder;
	}

I think that was added at a time where node:dns/promises did not exist and require('node:dns').promises was the only way of accessing the promise API. Would you be interested in sending a PR fixing this?

[F3n67u]

When you write fs/promises, do you mean dns/promises?

Yes. sorry about that. I messed fs/promises and dns/promises because I worked on both fs/promises and dns/promises recently.

I think that was added at a time where node:dns/promises did not exist and require('node:dns').promises was the only way of accessing the promise API. Would you be interested in sending a PR fixing this?

I am interested to contribute.

There's a bug in the code where properties are added to node:dns/promises only if someone tries to access require('node:dns').promises?

I am not clear what you propose to solve this problem. do you mean:

1. remove promises property of node:dns not lazy load, init it when node:dns is required instead of when access require('node:dns').promises

 const dnsException = errors.dnsException;
 
-let promises = null; // Lazy loaded
+let promises  = require('internal/dns/promises');
+promises.setServers = defaultResolverSetServers;
+promises.setDefaultResultOrder = setDefaultResultOrder;
 
 function onlookup(err, addresses) {
   if (err) {
@@ -374,22 +376,8 @@ module.exports = {
   NOTINITIALIZED: 'ENOTINITIALIZED',
   LOADIPHLPAPI: 'ELOADIPHLPAPI',
   ADDRGETNETWORKPARAMS: 'EADDRGETNETWORKPARAMS',
-  CANCELLED: 'ECANCELLED'
+  CANCELLED: 'ECANCELLED',
+  promises,
 };
 
 bindDefaultResolver(module.exports, getDefaultResolver());
-
-ObjectDefineProperties(module.exports, {
-  promises: {
-    configurable: true,
-    enumerable: true,
-    get() {
-      if (promises === null) {
-        promises = require('internal/dns/promises');
-        promises.setServers = defaultResolverSetServers;
-        promises.setDefaultResultOrder = setDefaultResultOrder;
-      }
-      return promises;
-    }
-  }
-});

2. or add properties(setServers, setDefaultResultOrder) in internal/dns/promises instead of in node:dns and keep promises variable of node:dns lazy load.

diff --git a/lib/dns.js b/lib/dns.js
index af5416c624..7557654563 100644
--- a/lib/dns.js
+++ b/lib/dns.js
@@ -386,8 +386,10 @@ ObjectDefineProperties(module.exports, {
     get() {
       if (promises === null) {
         promises = require('internal/dns/promises');
-        promises.setServers = defaultResolverSetServers;
-        promises.setDefaultResultOrder = setDefaultResultOrder;
+        // add setServers and setDefaultResultOrder in internal/dns/promises
+
+        // promises.setServers = defaultResolverSetServers;
+        // promises.setDefaultResultOrder = setDefaultResultOrder;
       }
       return promises;
     }

[aduh95]

I would go with the latter, keeping the promise API lay-loaded or not is not exactly relevant, the key thing is: the whole promise API should be fully constructed from lib/internal/dns/promises.js, it should not be split between lib/dns.js and lib/internal/dns/promises.js.

[F3n67u]

I agree with you. I will make a pr to fix this problem.

[F3n67u]

@aduh95 I made a pr to fix this problem, please take a loo: #43227