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

feat: add timeout option for both install and test phase #769

Merged
merged 6 commits into from
May 9, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ For syntax, see [lookup.json](./lib/lookup.json), the available attributes are:
"useGitClone": true Use a shallow git clone instead of downloading the module
"ignoreGitHead": Ignore the gitHead field if it exists and fallback to using github tags
"yarn": Install and test the project using yarn instead of npm
"testTimeout": Number of milliseconds before timeout
SimenB marked this conversation as resolved.
Show resolved Hide resolved
```

If you want to pass options to npm, eg `--registry`, you can usually define an
Expand Down
2 changes: 1 addition & 1 deletion bin/citgm-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ const options = {
failFlaky: app.failFlaky,
level: app.verbose,
npmLevel: app.npmLoglevel,
timeoutLength: app.timeout,
testTimeout: app.timeout,
tmpDir: app.tmpDir,
customTest: app.customTest,
yarn: app.yarn,
Expand Down
2 changes: 1 addition & 1 deletion bin/citgm.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const options = {
testPath: app.testPath,
level: app.verbose,
npmLevel: app.npmLoglevel,
timeoutLength: app.timeout,
testTimeout: app.timeout,
sha: app.sha,
tmpDir: app.tmpDir,
customTest: app.customTest,
Expand Down
3 changes: 1 addition & 2 deletions lib/common-args.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
description: 'Set timeout for npm install'
Copy link
Member Author

Choose a reason for hiding this comment

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

we use it for test now - maybe a dedicated test timeout makes sense?

})
.option('yarn', {
alias: 'y',
Expand Down
2 changes: 1 addition & 1 deletion lib/grab-project.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ async function grabProject(context) {
// Default timeout to 10 minutes if not provided
const timeout = setTimeout(
cleanup,
context.options.timeoutLength || 1000 * 60 * 10
context.options.testTimeout || 1000 * 60 * 10
);

function cleanup() {
Expand Down
3 changes: 3 additions & 0 deletions lib/lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ function resolve(context) {
if (rep.yarn) {
context.module.useYarn = true;
}
if (rep.testTimeout) {
context.module.testTimeout = rep.testTimeout;
}
context.module.flaky = context.options.failFlaky
? false
: isMatch(rep.flaky);
Expand Down
1 change: 1 addition & 0 deletions lib/package-manager/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ function install(packageManager, context) {

const proc = spawn(packageManagerBin, args, options);
const finish = timeout(
packageManager,
context,
proc,
(err) => {
Expand Down
9 changes: 8 additions & 1 deletion lib/package-manager/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,14 @@ async function test(packageManager, context) {
);

const proc = spawn(bin, args, options);
const finish = timeout(context, proc, runScript, 'Test');
const finish = timeout(
packageManager,
context,
proc,
runScript,
'Test',
context.module.testTimeout
);

proc.stdout.on('data', (data) => {
context.testOutput.append(data);
Expand Down
16 changes: 12 additions & 4 deletions lib/timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,17 @@
// Default timeout to 10 minutes if not provided
const kDefaultTimeout = 1000 * 60 * 10;

function timeout(context, proc, next, step) {
function timeout(
packageManager,
context,
proc,
next,
step,
moduleConfigTimeout
) {
let hasRun = false;
const delay = context.options.timeoutLength || kDefaultTimeout;
const delay =
context.options.testTimeout || moduleConfigTimeout || kDefaultTimeout;
// Third arg === `true` is the way to signal `finish` that this is a timeout.
// Otherwise it acts like a "regular" callback, i.e. `(err, ret) => {}`.
// `if (timedOut)` will overwrite `err` & `ret`, so first 2 args are ignored.
Expand All @@ -28,8 +36,8 @@ function timeout(context, proc, next, step) {
context.emit(
'data',
'error',
`${context.module.name} npm:`,
`npm-${step.toLowerCase()} Timed Out`
`${context.module.name} ${packageManager}:`,
`${packageManager}-${step.toLowerCase()} Timed Out`
);
proc.kill();
err = new Error(`${step} Timed Out`);
Expand Down
2 changes: 1 addition & 1 deletion test/npm/test-npm-install.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ test('npm-install: timeout', async (t) => {
sandbox,
{
npmLevel: 'silly',
timeoutLength: 100
testTimeout: 100
}
);
try {
Expand Down
2 changes: 1 addition & 1 deletion test/npm/test-npm-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ test('npm-test: timeout', async (t) => {
sandbox,
{
npmLevel: 'silly',
timeoutLength: 100
testTimeout: 100
}
);
try {
Expand Down
2 changes: 1 addition & 1 deletion test/test-grab-project.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ test('grab-project: timeout', async (t) => {
meta: {},
options: {
npmLevel: 'silly',
timeoutLength: 10
testTimeout: 10
}
};
try {
Expand Down
8 changes: 4 additions & 4 deletions test/test-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ test('timeout:', (t) => {
sandbox,
{
npmLevel: 'silly',
timeoutLength: 100
testTimeout: 100
}
);
const proc = {
Expand All @@ -39,7 +39,7 @@ test('timeout:', (t) => {
err = e;
ret = r;
};
const finish = timeout(context, proc, next, 'Tap');
const finish = timeout('npm', context, proc, next, 'Tap');
setTimeout(() => {
t.notOk(context.module.flaky, 'Time out should not mark module flaky');
t.equals(proc.killed, 1);
Expand All @@ -61,7 +61,7 @@ test('timeout:', (t) => {
sandbox,
{
npmLevel: 'silly',
timeoutLength: 100
testTimeout: 100
}
);
const proc = {
Expand All @@ -80,7 +80,7 @@ test('timeout:', (t) => {
ret = r;
return sentinel1;
};
const finish = timeout(context, proc, next, 'Tap');
const finish = timeout('npm', context, proc, next, 'Tap');
const r = finish(sentinel2, sentinel3);
t.equals(r, sentinel1);
t.equals(proc.killed, 0);
Expand Down
2 changes: 1 addition & 1 deletion test/yarn/test-yarn-install.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ test('yarn-install: timeout', async (t) => {
packageManagers,
sandbox,
{
timeoutLength: 100
testTimeout: 100
}
);
try {
Expand Down
2 changes: 1 addition & 1 deletion test/yarn/test-yarn-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ test('yarn-test: timeout', async (t) => {
packageManagers,
sandbox,
{
timeoutLength: 100
testTimeout: 100
}
);
try {
Expand Down