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: good-second-contribution — tracking tests that need some more 💘 #14544

Closed
3 tasks
refack opened this issue Jul 30, 2017 · 18 comments
Closed
3 tasks

test: good-second-contribution — tracking tests that need some more 💘 #14544

refack opened this issue Jul 30, 2017 · 18 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Jul 30, 2017 •
edited by gibfahn
Loading

A very biased list of tests that could enjoy some non-trivial refactoring

  1. Make test compliant with the test writing guide
  2. Make sure the test has a general description (PR or issue references most welcome)
  3. Make sure test cases are well separated (IIFE, functions, or just scopes) and documented
  4. Refactor away non-trivial micro-test-harnesses — example sequential/test-debugger-debug-brk.js
  5. Proper use of test/common utilities

new entries
@refack refack added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available test Issues and PRs related to the tests. labels Jul 30, 2017
@Trott
Copy link
Member

Trott commented Jul 30, 2017

#14541

@Trott Trott closed this as completed Jul 30, 2017
@refack refack mentioned this issue Jul 30, 2017
Closed
2 tasks
@refack refack reopened this Jul 30, 2017
@refack
Copy link
Contributor Author

refack commented Jul 30, 2017

#14541

I was thinking something more extensive, and obviously the "list" is going to get longer...

@refack
Copy link
Contributor Author

refack commented Jul 30, 2017

Like what you did in #14534

@refack
Copy link
Contributor Author

refack commented Jul 30, 2017

@nodejs/testing please add/remove/comment/edit

@shivanth
Copy link
Contributor

Can I take this up ?

@refack
Copy link
Contributor Author

refack commented Jul 31, 2017

Can I take this up ?

Sure. If you need any help, ping me (here or on IRC).
Also if you find any other tests that are written in an outdated style, post them here, and I'll add them to the list.

@oantoro
Copy link
Contributor

oantoro commented Aug 9, 2017

Hi @refack

I am looking for an issue for my first contribution. May be I can work on this?

@shivanth
Copy link
Contributor

shivanth commented Aug 9, 2017

@okyantoro @refack A little with held upwith stuffs, Feel free to reassign this

@oantoro
Copy link
Contributor

oantoro commented Aug 9, 2017

I observed some test files and I found parallel/test-http-methods.js only covered GET, HEAD and POST and the last case compares the http.METHODS with http.METHODS.sort()
My question is why is just those three methods covered? Is it okay to add cases to check existence of other methods like PUT, DELETE etc?
Thanks

@Trott
Copy link
Member

Trott commented Aug 10, 2017

Is it okay to add cases to check existence of other methods like PUT, DELETE etc?

Assuming it isn't duplicating another test that we already have, more test coverage is 👍

oantoro added a commit to oantoro/node that referenced this issue Aug 11, 2017
@oantoro
test: add case to test-http-methods.js
30ea061 
Add more case to check existence of DELETE and PUT methods.

Refs: nodejs#14544
@oantoro oantoro mentioned this issue Aug 11, 2017
Closed
2 tasks
aqrln pushed a commit that referenced this issue Aug 14, 2017
@oantoro @aqrln
test: cover all HTTP methods that parser supports
1268737 
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Aug 14, 2017
@oantoro @addaleax
test: cover all HTTP methods that parser supports
6d9f94f 
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@benhalverson
Copy link
Member

Is this still being worked on? @refack @Trott
I'd like to help.

MylesBorins pushed a commit that referenced this issue Sep 20, 2017
@oantoro @MylesBorins
test: cover all HTTP methods that parser supports
1aac05b 
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jonask-wix
Copy link
Contributor

hi, one of the links in issue is broken, it points to https://github.com/nodejs/node/blob/master/test/sequential/test-domain-abort-on-uncaught.js

@gibfahn
Copy link
Member

gibfahn commented Nov 6, 2017

@jonask-wix fixed.

nikniv added a commit to nikniv/node that referenced this issue Nov 28, 2017
@nikniv
test: expanded assertions for console.timeEnd() output
a5ac0e3 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve nodejs#14544 but will
not address the whole thing.

Refs: nodejs#14643
@nikniv nikniv mentioned this issue Nov 28, 2017
Closed
3 tasks
@nikniv
Copy link
Contributor

nikniv commented Dec 4, 2017 •
edited
Loading

Hey @refack, the following point has been done in PR #17368 :

parallel/test-console.js - after #14643 lands this test should assert the expected outputs. Currently it only checks that values are written to stdout and don't throw.

Also, please open this issue again since refactoring of other files still needs to be done. Thanks! 🙂

@gibfahn gibfahn reopened this Dec 4, 2017
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@nikniv @MylesBorins
test: expanded assertions for console.timeEnd() output
efde4b7 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@nikniv @MylesBorins
test: expanded assertions for console.timeEnd() output
ee1c95f 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@juggernaut451
Copy link
Contributor

hello @NiveditN can you please tell which all files are to be refactored. As i would like to help on this

@juggernaut451 juggernaut451 mentioned this issue Feb 12, 2018
Closed
4 tasks
@juggernaut451
Copy link
Contributor

juggernaut451 commented Feb 13, 2018 •
edited
Loading

Anyone available for mentoring me regarding this issue? Would like to contribute
@refack @Trott @MylesBorins

@BridgeAR
Copy link
Member

@juggernaut451 a lot of our tests do not contain a description what they actually test. That would for example be a good thing to add.

Another thing that I am aware of is that our documentation lacks a lot of "changed" entries. The latter is probably quite some work though to check all the changes. Even though you probably only have to check the concrete API history and check when what API changed and if a changed entry was added or not.

@BridgeAR
Copy link
Member

Please also look at https://www.nodetodo.org/next-steps/

This was referenced Feb 14, 2018
Closed
Closed
Closed
@juggernaut451 juggernaut451 mentioned this issue Mar 2, 2018
Closed
2 tasks
@juggernaut451 juggernaut451 mentioned this issue Mar 2, 2018
Closed
2 tasks
juggernaut451 added a commit to juggernaut451/node that referenced this issue Mar 7, 2018
@juggernaut451
test: refactor parallel/test-tls-ca-concat.js
ee77247 
Implementing common.mustCall and changing the function to arrow function

Fixes : nodejs#14544
juggernaut451 added a commit to juggernaut451/node that referenced this issue Mar 7, 2018
@juggernaut451
test: refactor parallel/test-tls-cert-chains-concat
8a11196 
added common.mustCall and replaced function with arrow function

Fixes : nodejs#14544
targos pushed a commit that referenced this issue Mar 27, 2018
@juggernaut451 @targos
test: refactor parallel/test-tls-ca-concat.js
0310df8 
PR-URL: #19092
Fixes: #14544
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Apr 9, 2018
@juggernaut451 @BridgeAR
test: refactor parallel/test-tls-cert-chains-concat
cbc7eb7 
Added common.mustCall and replaced function with arrow function.

PR-URL: nodejs#19096
Fixes: nodejs#14544
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this issue Apr 12, 2018
@juggernaut451 @targos
test: refactor parallel/test-tls-cert-chains-concat
6613169 
Added common.mustCall and replaced function with arrow function.

PR-URL: #19096
Fixes: #14544
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 3, 2018
@juggernaut451 @BethGriggs
test: refactor parallel/test-tls-ca-concat.js
146c488 
PR-URL: #19092
Fixes: #14544
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@refack @shivanth @Trott @juggernaut451 @oantoro @benhalverson @BridgeAR @jonask-wix @nikniv @gibfahn