GeezerVison

nit: this could be simplified as mustCall will check that the callback is only invoked once. We can also explicitly provide the number of calls we are expecting so it's clearer:

@aduh95 Your suggestion is simpler, but is slightly more permissive than the code in this PR. The code in this PR guarantees that the first call of the API results in a warning and the second call does not. The change you suggest here will still result in a passing test if the first call does not result in a warning and the second one does. Admittedly, that's an edge case that is unlikely to happen. But I have a slight preference for the stricter checking here.

I don’t see how, the current code checks the 'warning' event is emitted once, but I don’t think it can know what API call triggered it either 🤔

I don’t see how, the current code checks the 'warning' event is emitted once, but I don’t think it can know what API call triggered it either 🤔

Current code in master branch tests it by having two separate tests that are nearly identical. One test only calls the API once and makes sure that the warning is emitted once. The other test calls the API twice and makes sure that the warning is emitted once. Alone, that second test doesn't confirm that the warning happens on the first API call, but in combination with the first test, that is effectively checked.

This PR consolidates those two tests into a single test.

Oh, wait, or are you suggesting that there will be a problem if something else (like --pending-deprecations) causes warnings? Yeah, that's a valid concern and one we should address....

I'm wondering if we're actually best off just having two tests. :-D

@kuriyosh How would you feel about restoring both tests and adding a short comment in each one explaining why there are two different-but-very-similar tests?

In my understanding, the discussed concern is that this test fails if other warnings occur in a second call. (Please point me out if I'm wrong here.) I agree that we need to run test-net-deprecated-setsimultaneousaccepts.js to make sure that _setSimultaneousAccepts() does not emit other warnings.

On the other hand, test-net-server-simultaneous-accepts-produce-warning-once.js in master branch does not check whether if _setSimultaneousAccepts() emit DEP0121 in a second call. That test can be passed if second calling _setSimultaneousAccepts() emit DEP0121 because of the behavior of expectsWarning().
So, I think the change of test-net-server-simultaneous-accepts-produce-warning-once.js in this PR is needed.

On the other hand, test-net-server-simultaneous-accepts-produce-warning-once.js in master branch does not check whether if _setSimultaneousAccepts() emit DEP0121 in a second call.

It does check. expectsWarning() will fail if it receives the warning more than once.

This exits with a failure status:

const {expectWarning} = require('./test/common');

expectWarning('foo', 'foo', 'foo');

This exits with success status:

const {expectWarning} = require('./test/common');

expectWarning('foo', 'foo', 'foo');

process.emitWarning('foo', 'foo', 'foo');

And this exits with a failure status again:

const {expectWarning} = require('./test/common');

expectWarning('foo', 'foo', 'foo');

process.emitWarning('foo', 'foo', 'foo');
process.emitWarning('foo', 'foo', 'foo');

One thing I notice while running these tests is that the error message is not great on that last example because const [ message, code ] = expected.shift(); (in test/common/index.js) does not accommodate the possibility that expected is an empty array and expected.shift() returns undefined:

TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))

So that's probably worth fixing.

So that's probably worth fixing.

#41326

Sorry, I misunderstood the behavior of expectWarning...
I totally agree on having two separate tests.
I have committed for adding comment in these test codes.