Skip to content
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

Jump to bottom

test: remove duplicate test about warning in lib/net.js #41307

Merged
merged 4 commits into from
Dec 27, 2021
Merged

test: remove duplicate test about warning in lib/net.js #41307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fa3e4e
test: remove duplicate test about deprecation warning
kuriyosh Dec 24, 2021
c23d0e6
test: revert two test and add comments for each
kuriyosh Dec 26, 2021
2045739
test: fix lint error
kuriyosh Dec 26, 2021
6ef49ff
test: update comment for test description
kuriyosh Dec 27, 2021
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
16 changes: 0 additions & 16 deletions test/parallel/test-net-deprecated-setsimultaneousaccepts.js
View file
Open in desktop

This file was deleted.

5 changes: 4 additions & 1 deletion test/parallel/test-net-server-simultaneous-accepts-produce-warning-once.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
'use strict';

const { expectWarning } = require('../common');
const { expectWarning, mustCall, mustNotCall } = require('../common');
const net = require('net');

expectWarning(
'DeprecationWarning',
'net._setSimultaneousAccepts() is deprecated and will be removed.',
'DEP0121');

Trott marked this conversation as resolved.
Show resolved Hide resolved
process.on('warning', mustCall(() => {
process.on('warning', mustNotCall());
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
process.on('warning', mustCall(() => {
process.on('warning', mustNotCall());
}));
process.on('warning', mustCall(1));

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Trott Trott Dec 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

So that's probably worth fixing.

#41326

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

net._setSimultaneousAccepts();
net._setSimultaneousAccepts();