doc: edit writing-tests.md #10585
doc: edit writing-tests.md #10585
Conversation
Trott
added
doc
| by `common` are used, it can be included without assigning it to an identifier: | ||
| Even if a test uses no functions or other properties exported by `common`, | ||
| the test should still include the `common` module. This is because the `common` | ||
| module includes code that will cause tests to fail if there are variables leaked |
There was a problem hiding this comment.
Why the passive->active change here? Is this a general rule we follow? I find if there are variables leaked into the global space less clear than if variables are leaked into the global space.
There was a problem hiding this comment.
How about if the test leaks variables into the global space?
The passive->active change is just following a general writing recommendation for increased clarity, but of course we should stick to passive voice if changing to active does not increase clarity. :-D
| depending on the platform. For example: | ||
|
|
||
| ```javascript | ||
| const timer = setTimeout(fail, common.platformTimeout(4000)); | ||
| ``` | ||
|
|
||
| will create a 4-seconds timeout, except for some platforms where the delay will | ||
| be multiplied for some factor. | ||
| will create a 4-seconds timeout on most platforms but a longer timeout on slower |
| the global space. In situations where no functions or other properties exported | ||
| by `common` are used, it can be included without assigning it to an identifier: | ||
| Even if a test uses no functions or other properties exported by `common`, | ||
| the test should still include the `common` module. This is because the `common` |
There was a problem hiding this comment.
"include the common module, as the first executable statement" maybe?
There was a problem hiding this comment.
Or maybe to be really clear just say that the first two lines of the test must be:
'use strict';
const common = require('../common');or
'use strict';
require('../common');There was a problem hiding this comment.
Generally, this holds good. But in some tests we have comments at the beginning (I mean before common) and that is also acceptable.
There was a problem hiding this comment.
Is there a reason we couldn't just standardise on what's in this guide?
'use strict';
require('../common');
// Comments after thisThere was a problem hiding this comment.
If we did that we could lint for it.
There was a problem hiding this comment.
Lets see what @Trott thinks about it.
There was a problem hiding this comment.
How about include the common module before any other modules. as a reasonable concise compromise that we can expand on later if it is found to be inadequate?
|
Ping @Trott |
|
Nits addressed, I think. |
* Remove passive voice * Remove unneeded modifiers * Minor comma change
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
|
Landed in 0674789 |
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: nodejs#10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: #10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
* Remove passive voice * Remove unneeded modifiers * Minor comma change PR-URL: #10585 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>
Checklist
Affected core subsystem(s)
doc test