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: More descriptive name for test file names #19592

Closed
dsinecos opened this issue Mar 25, 2018 · 5 comments
Closed

test: More descriptive name for test file names #19592

dsinecos opened this issue Mar 25, 2018 · 5 comments
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.

Comments

@dsinecos
Copy link
Contributor

  • Version: master
  • Platform: 16.04.1, Ubuntu
  • Subsystem: test

I was going through the following test files test-process-geteuid-getegid.js and test-process-setuid-setgid.js

I was wondering if the filenames could include the name of all the methods that are being tested so

  • test-process-geteuid-getegid.js could be renamed to test-process-geteuid-getegid-seteuid-setegid.js
  • And test-process-setuid-setgid.js could be renamed to test-process-getuid-getgid-setuid-setgid.js

I had a little confusion when locating these files and I thought maybe this change would help. Also, I read the guidelines on naming test files and it allows for listing the methods being tested and additional information where needed.

There are test files with over 60 characters eg. test-timers-socket-timeout-removes-other-socket-unref-timer.js, test-process-exception-capture-should-abort-on-uncaught-setflagsfromstring.js so I understand the renaming would not violate any character limit restrictions on test file names

If the above change isn't possible I was thinking for the sake of consistency, the test files could be renamed to test-process-getuid-getgid.js and test-process-getegid-geteuid.js. I was initially confused for a bit when I found the test-process-geteuid-getegid.js but not a corresponding test-process-getuid-getgid.js

@tniessen
Copy link
Member

Even though there is no hard limit for the file name length, it is usually a good idea to keep them as short as reasonably possible. How about test-process-euid-egid.js and test-process-uid-gid.js?

@dsinecos
Copy link
Contributor Author

dsinecos commented Mar 25, 2018

@tniessen That makes sense, this way it keeps the file names concise and also indicates what kind of methods the file tests for

@lpinca lpinca added test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. labels Apr 2, 2018
@rajdhandus
Copy link
Contributor

@dsinecos @tniessen @lpinca - I would like to take this up.. Can i please work on this issue?

@lpinca
Copy link
Member

lpinca commented Apr 2, 2018

@rajdhandus sure.

@rajdhandus
Copy link
Contributor

thanks..

rajdhandus added a commit to rajdhandus/node that referenced this issue Apr 6, 2018
test-process-geteuid-getegid.js will be renamed to
test-process-euid-egid.js

test-process-setuid-setgid.js will be renamed to
test-process-uid-gid.js

Fixes: nodejs#19592
targos pushed a commit that referenced this issue Apr 12, 2018
test-process-geteuid-getegid.js is renamed to test-process-euid-egid.js.

test-process-setuid-setgid.js is renamed to test-process-uid-gid.js.

PR-URL: #19765
Fixes: #19592
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants