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

lookup: declare bankruptcy on flaky modules #959

Merged
merged 3 commits into from
Sep 24, 2023
Merged

lookup: declare bankruptcy on flaky modules #959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f18997b
lookup: declare bankruptcy on flaky modules
Trott Sep 7, 2023
0693e0f
fixup! lookup: declare bankruptcy on flaky modules
Trott Sep 24, 2023
ebe04c6
fixup! fixup! lookup: declare bankruptcy on flaky modules
Trott Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 14 additions & 57 deletions lib/lookup.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@
"prefix": "v",
"skip": ["win32", "aix"]
},
"bluebird": {
"prefix": "v",
"maintainers": "petkaantonov"
},
"body-parser": {
"flaky": "aix",
"maintainers": "dougwilson"
Expand Down Expand Up @@ -90,7 +86,7 @@
"maintainers": "mcollina",
"flaky": ["rhel"],
"prefix": "v",
"skip": ["aix", "win32"]
"skip": ["aix", "darwin", "win32"]
},
"coffeescript": {
"maintainers": ["jashkenas", "GeoffreyBooth"],
Expand Down Expand Up @@ -153,19 +149,10 @@
"expectFail": "fips",
"skip": ["win32"]
},
"express": {
"flaky": "ppc",
"maintainers": "dougwilson",
"skip": "win32"
},
"express-session": {
"prefix": "v",
"maintainers": "dougwilson"
},
"fastify": {
"maintainers": ["mcollina", "delvedor"],
"prefix": "v"
},
"flush-write-stream": {
"prefix": "v",
"maintainers": "mafintosh",
Expand Down Expand Up @@ -214,10 +201,6 @@
"maintainers": "contra",
"skip": "win32"
},
"https-proxy-agent": {
"maintainers": "TooTallNate",
"scripts": ["build", "test"]
},
"iconv": {
"prefix": "v",
"flaky": "aix",
Expand All @@ -235,6 +218,7 @@
},
"is-core-module": {
"prefix": "v",
"skip": "win32",
"maintainers": "ljharb",
"scripts": ["tests-only"]
},
Expand Down Expand Up @@ -310,11 +294,13 @@
},
"mime": {
"prefix": "v",
"maintainers": "broofa"
"maintainers": "broofa",
"skip": "win32"
},
"minimist": {
"npm": true,
"maintainers": "substack"
"skip": "win32",
"maintainers": "ljharb"
},
"mkdirp": {
"head": true,
Expand Down Expand Up @@ -358,11 +344,6 @@
"comment": "Flaky because of test timeouts",
"skip": true
},
"npm": {
"maintainers": ["nodejs/npm"],
"prefix": "v",
"skip": ["aix", "s390"]
},
"path-to-regexp": {
"prefix": "v",
"maintainers": "blakeembrey",
Expand All @@ -386,11 +367,6 @@
"comment": "Error message changes in V8 9.3",
"repo": "https://github.com/pugjs/pug"
},
"pump": {
"prefix": "v",
"maintainers": "mafintosh",
"skip": "win32"
},
"pumpify": {
"prefix": "v",
"maintainers": "mafintosh",
Expand Down Expand Up @@ -426,17 +402,8 @@
},
"rewire": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this: there is no issue with Node 20. The test passes on GitHub. The tests have been working fine for a couple of years. The test is also not asynchronous or contains any randomness.

The failing test is testing the TypeScript integration, which took 300ms on my machine and takes 1.2s on GitHub. My assumption is that the test timeout of 2s is too aggressive for more expensive tests in combination with a busy CPU.

What's the recommendation in that case? How can I address this? :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any recommendation? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the "script" setting in CITGM to pass a longer timeout via CLI to mocha. Like maybe npm run test --timeout 5000 will work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try that and run CITGM and see what happens.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, passing npm run test -- --timeout 5000 works. I've just tested it :)

"prefix": "v",
"maintainers": "jhnns"
},
"rimraf": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rimraf: Failing because ts-node doesn't work on node 20.

"prefix": "v",
"flaky": "win32",
"maintainers": "isaacs"
},
"router": {
"prefix": "v",
"maintainers": "dougwilson",
"skip": "win32"
"maintainers": "jhnns",
"scripts": ["test -- --timeout 30000"]
},
"sax": {
"skip": "win32",
Expand All @@ -454,10 +421,8 @@
},
"serialport": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our tests work reliably on node20, runtime libuv however not so much but it would not be seen here as it needs hardware to run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like serialport's failures are due to macos disk space - happy to drop macos from the citgm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serialport is one of the few where we probably don't want to skip macOS if we can avoid it, but certainly skipping that one OS is better than complete removal. I'll add it back in and also remove the skip for Windows and run a CITGM. If it fails on Windows and/or macOS but nowhere else, we'll skip those but leave it alone otherwise. (Sorry our macOS test machines are not as robustly provisioned as we might like. Alas, we run on donated services, etc.)

"prefix": "serialport@",
"flaky": ["ppc", "rhel"],
"tags": "native",
"maintainers": "reconbot",
"skip": ["win32"]
"maintainers": "reconbot"
},
"socket.io": {
"maintainers": "rauchg",
Expand Down Expand Up @@ -497,7 +462,9 @@
"tape": {
"head": true,
"prefix": "v",
"maintainers": "substack"
"skip": "win32",
"scripts": ["tests-only"],
"maintainers": "ljharb"
},
"thread-sleep": {
"install": ["install", "--build-from-source"],
Expand All @@ -521,9 +488,6 @@
"prefix": "v",
"maintainers": "mafintosh"
},
"tough-cookie": {
"maintainers": ["awaterma", "colincasey", "ruoho", "wjharney"]
},
"uglify-js": {
"prefix": "v",
"flaky": ["ppc", "darwin"],
Expand All @@ -537,10 +501,6 @@
"maintainers": "jashkenas",
"ignoreGitHead": true
},
"undici": {
"prefix": "v",
"maintainers": ["mcollina", "ronag"]
},
"uuid": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just release uuid@v9.0.1 which should fix the test failure. Can you add it back in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it back in and re-run CITGM. 🤞

"prefix": "v",
"maintainers": ["ctavan", "broofa"],
Expand Down Expand Up @@ -568,13 +528,10 @@
"maintainers": "tootallnate",
"tags": "native"
},
"winston": {
"flaky": ["win32", "ppc", "s390", "darwin"],
"maintainers": "indexzero"
},
"ws": {
"expectFail": "fips",
"maintainers": ["einaros", "3rd-Eden", "lpinca"]
"maintainers": ["einaros", "3rd-Eden", "lpinca"],
"scripts": ["test -- --timeout 30000"]
},
"yargs": {
"comment": "Install from source is currently broken due to TS error",
Expand Down
2 changes: 1 addition & 1 deletion lib/package-manager/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export async function test(packageManager, context) {
scripts = context.module.scripts.map((script) => [
packageManagerBin,
'run',
script
...script.split(' ')
]);
} else {
scripts = [[packageManagerBin, 'test']];
Expand Down
5 changes: 1 addition & 4 deletions test/test-lookup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,7 @@ test('lookup[getLookupTable]:', (t) => {
});
t.ok(table, 'table should exist');
t.ok(table.lodash, 'lodash should be in the table');
t.ok(
table.underscore.maintainers,
'underscore should contain a maintainers parameter'
);
t.ok(table.weak.maintainers, 'weak should contain a maintainers parameter');
t.end();
});

Expand Down