-
Notifications
You must be signed in to change notification settings - Fork 30k
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: fix require nits in some test-tls-* tests #14008
Conversation
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names.
CI: https://ci.nodejs.org/job/node-test-pull-request/8900/ All fails seem unrelated. |
|
||
const assert = require('assert'); | ||
const { spawn } = require('child_process'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, the test writing guide says:
The require statements are sorted in ASCII order (digits, upper case, _, lower case).
It's not clear to me if this refers to the variable names or the module names. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sam-github? Ref: 02b9270
Since the whole paragraph's subject is modules, my intuition says sort by module name.
(also congruent with Jewish law that states you bless food according to the main ingredient and not by how you eat it 😉)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think module name sorting would be more consistent here, as we can use many variable names in destructuring and only one module name. See examples.
|
Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/8967/ |
Landed in cc1a47d |
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Do not require if test is skipped. * Do not re-require without need. * Sort requiring by module names. PR-URL: #14008 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test
require
if test is skipped.require
without need.I've just stumble upon these nits in these connected tests by chance. Maybe I shall try to fix this class of nits in tests more thoroughly later if this makes any sense.