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.

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

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

Merged
merged 4 commits into from
Dec 27, 2021

Conversation

kuriyosh
Copy link
Contributor

test-net-server-simultaneous-accepts-produce-warning-once.js test a same code as test-net-deprecated-setsimultaneousaccepts.js.
So I remove test-net-server-simultaneous-accepts-produce-warning-once.js.

before coverage:
https://coverage.nodejs.org/coverage-895c3d937ea74faa/lib/net.js.html#L1761

after coverage:
Screen Shot 2021-12-24 at 11 38 37

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 24, 2021
@kuriyosh kuriyosh changed the title test: remove duplicate test about deprecation warning test: remove duplicate test about warning in lib/net.js Dec 24, 2021
@Trott
Copy link
Member

Trott commented Dec 24, 2021

These tests are not quite the same. The test you left makes sure that the warning is emitted on the first call. The test you deleted makes sure that the warning is emitted only once even when the API is called twice. They can certainly be consolidated into a single test, but they do not quite test the same things.

@kuriyosh
Copy link
Contributor Author

@Trott Thanks for your comment!
I didn't understand the purpose of these tests correctly.

Based on your comment, I think it is test-net-server-simultaneous-accepts-produce-warning-once.js that should be left.
Because, by running test-net-server-simultaneous-accepts-produce-warning-once.js, we can check if the warning is emitted and check the behavior of the second execution at the same time.
Could you review my PR again?

after coverage:
Screen Shot 2021-12-24 at 13 53 00

@Trott
Copy link
Member

Trott commented Dec 24, 2021

Because, by running test-net-server-simultaneous-accepts-produce-warning-once.js, we can check if the warning is emitted and check the behavior of the second execution at the same time. Could you review my PR again?

I've added some suggestions to try to make sure that the warning is called on the first invocation of the API and not on the second. (The way the test is written right now, I think it only checks that the warning is raised once but does not confirm it was the first call and not the second call. That's fine if the other test exists because the other tests confirms the warning is emitted on the first call. But if we're removing that other test, then maybe we should check that here too.)

@kuriyosh
Copy link
Contributor Author

Thank you for your smart solution!
@Trott 's code exactly check that DEP0121 is emitted only once.
I totaly agree with your suggestion.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2021
Comment on lines 11 to 13
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

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.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Comment on lines 4 to 5
// This test checks that `_setSimultaneousAccepts` emit DEP0121 warning.
// Ref test-net-server-simultaneous-accepts-produce-warning-once.js
Copy link
Member

@Trott Trott Dec 26, 2021

Choose a reason for hiding this comment

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

Suggested change
// This test checks that `_setSimultaneousAccepts` emit DEP0121 warning.
// Ref test-net-server-simultaneous-accepts-produce-warning-once.js
// Test that DEP0121 is emitted on the first call of _setSimultaneousAccepts().

@Trott
Copy link
Member

Trott commented Dec 27, 2021

I don't see a label for this but the commit queue should not be used to land this because the commit message for the first commit in this PR is no longer accurate. (@kuriyosh If you want to force-push an updated commit message, or even squash the four commits here into a single commit, that would be great. But if not, it's no big deal for someone to do it when landing this.)

@aduh95 aduh95 merged commit 24f4f7f into nodejs:master Dec 27, 2021
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41307
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
PR-URL: #41307
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41307
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41307
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants