Skip to content

Conversation

@ryzokuken
Copy link
Contributor

@ryzokukenryzokuken commented Mar 13, 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

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

Checklist

  • test/parallel/test-regress-GH-io-1068.js
  • test/parallel/test-regress-GH-io-1811.js
  • test/parallel/test-regress-GH-node-9326.js

Additional files renamed

  • timers-regress-GH-9765
  • test-tls-pfx-gh-5100-regr
  • test-tls-regr-gh-5108

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Mar 13, 2018
@ryzokuken
Copy link
ContributorAuthor

PSA: Naming commits poorly on purpose so that it's easier to rebase it in the end.

@joyeecheung
Copy link
Member

@lpincalpinca added the wip Issues and PRs that are still a work in progress. label Mar 14, 2018
@lpinca
Copy link
Member

@ryzokuken added "in progress" label as I assumed you want to rename more tests from the checklist in the description. Let me know If I'm wrong.

@ryzokuken
Copy link
ContributorAuthor

ryzokuken commented Mar 15, 2018

@lpinca you're quite right, thanks. Sorry for not being able to reply immediately.

@ryzokuken
Copy link
ContributorAuthor

@joyeecheung the original 3 have been fixed and that ends the list in #19105. Thus, I'm adding the "Fixes: ..." reference, let me know if I shouldn't.

@lpincalpinca removed the wip Issues and PRs that are still a work in progress. label Mar 16, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please use "Node.js"?

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

Choose a reason for hiding this comment

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

Same thing.

@ryzokuken
Copy link
ContributorAuthor

@lpinca done.

@lpinca
Copy link
Member

@lpincalpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 16, 2018
Copy link
Member

@lpincalpincaMar 16, 2018

Choose a reason for hiding this comment

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

@ryzokuken it seems that the order of these requires was part of the test. This test is now failing on all machines. Please revert this particular change.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

My bad. Fixing this in a sec.

@lpincalpinca removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 16, 2018
@ryzokuken
Copy link
ContributorAuthor

@lpinca done. I shouldn't have meddled with it in the first place.

@lpinca
Copy link
Member

@ryzokuken
Copy link
ContributorAuthor

@lpinca they passed! 🎉

@lpincalpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018
@lpinca
Copy link
Member

@ryzokuken would you mind squashing commits? Thank you.

@ryzokuken
Copy link
ContributorAuthor

@lpinca sure! Just give me a second.

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 1. Renamed test-regress-GH-io-1068 to test-tty-stdin-end 2. Renamed test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror 3. Renamed test-regress-GH-node-9326 to test-kill-segfault-freebsd 4. Renamed test-timers-regress-nodejsGH-9765 to test-timers-setimmediate-infinite-loop 5. Renamed test-tls-pfx-nodejsgh-5100-regr to test-tls-pfx-authorizationerror 6. Renamed test-tls-regr-nodejsgh-5108 to test-tls-tlswrap-segfault Fixes: nodejs#19105 Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
@ryzokuken
Copy link
ContributorAuthor

@lpinca looks good enough?

@lpinca
Copy link
Member

@ryzokuken
Copy link
ContributorAuthor

@lpinca any idea what could've gone wrong?

@lpinca
Copy link
Member

AIX failure is not related, will land this later.

@lpinca
Copy link
Member

Landed in d54e0f8.

lpinca pushed a commit that referenced this pull request Mar 18, 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. - Rename test-regress-GH-io-1068 to test-tty-stdin-end - Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror - Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd - Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop - Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror - Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault PR-URL: #19332Fixes: #19105 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@lpincalpinca closed this Mar 18, 2018
@lpinca
Copy link
Member

There are a few more that weren't listed in the tracking issue:

test/parallel/test-arm-math-exp-regress-1376.js test/parallel/test-buffer-regression-649.js test/parallel/test-dgram-regress-4496.js test/parallel/test-dns-regress-7070.js test/parallel/test-http-agent-maxsockets-regress-4050.js 

@ryzokuken
Copy link
ContributorAuthor

@lpinca Indeed! I'd be making another PR covering these shortly. Thanks.

@lpinca
Copy link
Member

@ryzokuken awesome, thank you.

MylesBorins pushed a commit that referenced this pull request Mar 20, 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. - Rename test-regress-GH-io-1068 to test-tty-stdin-end - Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror - Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd - Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop - Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror - Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault PR-URL: #19332Fixes: #19105 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 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. - Rename test-regress-GH-io-1068 to test-tty-stdin-end - Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror - Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd - Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop - Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror - Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault PR-URL: #19332Fixes: #19105 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@targostargos mentioned this pull request Mar 20, 2018
@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
BethGriggs pushed a commit that referenced this pull request Dec 3, 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. - Rename test-regress-GH-io-1068 to test-tty-stdin-end - Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror - Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd - Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop - Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror - Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault PR-URL: #19332Fixes: #19105 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Dec 4, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@ryzokuken@joyeecheung@lpinca@jasnell@Leko@hiroppy@richardlau@gireeshpunathil@starkwang@tniessen@BethGriggs@nodejs-github-bot