lookup: add jsdom #740
lookup: add jsdom #740
Conversation
(cherry picked from commit e8428e8)
Codecov Report
@@ Coverage Diff @@
## master #740 +/- ##
==========================================
- Coverage 96.22% 96.11% -0.11%
==========================================
Files 27 27
Lines 874 876 +2
==========================================
+ Hits 841 842 +1
- Misses 33 34 +1
Continue to review full report at Codecov.
|
| @@ -73,8 +73,7 @@ module.exports = function commonArgs(app) { | |||
| .option('timeout', { | |||
| alias: 'o', | |||
| type: 'number', | |||
| description: 'Set timeout for npm install', | |||
| default: 1000 * 60 * 10 | |||
There was a problem hiding this comment.
Should the default not be kept / how come the removal is necessary?
There was a problem hiding this comment.
If the default is set at this point it overrides the module-specific timeout later on at https://github.com/nodejs/citgm/pull/740/files#diff-166959275b22802c268f62f152801cbcR9.
If --timeout is not provided, it falls back to kDefaultTimeout (which previously seems to have been redundant).
|
One thing I forgot to mention earlier is that some of the tests utilize a Python server to act as the 'backend'. Is Python available out of the box on all platforms? Would you like me to skip AIX here due to #688, or would you rather resolve it later on by automatically skipping AIX for all packages where Yarn is used? I also think I might need some help writing the tests for |
jsdom is a JavaScript implementation of the DOM and HTML standards, commonly used for testing and scraping websites while behaving much like a regular web browser.
It fulfills all the hard requirements of citgm (after the tests-in-tarball requirement was relaxed in #695) and several soft requirements given its widespread usage within testing frameworks. The code makes frequent use of less common APIs such as
vm, and is currently covered by around ~4500 tests that have revealed unusual bugs in the past.The tests usually take between 10-12 minutes to complete, so I've built on @SimenB's WIP to add the timeoutLength option in #728 with a few more changes in order for it to work.