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: rename regression tests with descriptive file names (pt. 6) #19608

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Mar 26, 2018

Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conform to the standard test structure

Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

Checklist

P.S. Two commits (f84a6b7 and 2ee8d87) are unrelated and cancel each other out. They won't matter after we squash.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 26, 2018
require('../common');
const Countdown = require('../common/countdown');

// This test ensures Node.js doesn't behave erratically when recieving pipelined
Copy link
Member

Choose a reason for hiding this comment

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

Nit: receiving

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's time I installed vscode-spell-check 😛

const common = require('../common');

// This test ensures that Node.js doesn't crash with an AssertionError at
// `ServerResponse.resOnFinish`
Copy link
Member

Choose a reason for hiding this comment

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

More helpful would be a comment that says what made this case special. (E.g., "This test ensures that Node.js deals with a large number of HTTP requests correctly." or some such.) Ditto with the test file name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks.

@TimothyGu
Copy link
Member

@ryzokuken
Copy link
Contributor Author

@TimothyGu done. Please take a look, especially at the comment and let me know if it's satisfactory.

Rename test-dgram-regress-4496 to test-dgram-typeerror-buffer to test-dgram-send-invalid-msg-type
Rename test-http-regr-nodejsgh-2821 to test-http-request-large-payload
Rename test-child-process-fork-regr-nodejsgh-2847 to test-child-process-fork-closed-channel-segfault
Rename test-http-pipeline-regr-2639 to test-http-pipeline-serverresponse-assertionerror
Rename test-http-pipeline-regr-3332 to test-http-pipeline-requests-connection-leak
Rename test-http-pipeline-regr-3508 to test-http-pipeline-socket-parser-typeerror

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
@devsnek
Copy link
Member

devsnek commented Mar 28, 2018

@trivikr
Copy link
Member

trivikr commented Mar 28, 2018

Landed in ff7c2cc

@trivikr trivikr closed this Mar 28, 2018
trivikr pushed a commit that referenced this pull request Mar 28, 2018
targos pushed a commit that referenced this pull request Apr 2, 2018
@targos targos mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants