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

Runtime: better errors #728

Merged
merged 14 commits into from
Jul 5, 2024
20 changes: 5 additions & 15 deletions packages/runtime/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,20 +69,6 @@ export class RTError extends Error {

// automatically limit the stacktrace (?)
Error.captureStackTrace(this, RTError.constructor);

// Provide custom rendering of the error in node
// TODO we should include some kind of context where it makes sense
// eg, if the error is associated with a job, show the job code
// eg, if the error came from an expression, show the source and location
// eg, if this came from our code, it doesn't help the user to see it but it does help us!
// @ts-ignore
this[util.inspect.custom] = (_depth, _options, _inspect) => {
const str = `[${this.name}] ${this.message}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This turned out to be a major cause of information getting lost :(


// TODO include stack trace if the error demands it

return str;
};
}
}

Expand Down Expand Up @@ -165,15 +151,19 @@ export class AdaptorError extends RTError {
name = 'AdaptorError';
severity = 'fail';
message: string = '';
details: any;
constructor(error: any) {
super();

this.details = error;
if (typeof error === 'string') {
this.message = error;
} else if (error.message) {
this.message = error.message;
}
}
// toString() {
// return util.inspect(this.details);
// }
}

// custom user error trow new Error() or throw {}
Expand Down
27 changes: 17 additions & 10 deletions packages/runtime/src/util/log-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,26 @@ export type ErrorReporter = (
}
) => ErrorReport;

const serialize = (error: any) => {
if (error instanceof Error) {
const result: any = {};
for (const key in error) {
// @ts-ignore
result[key] = serialize(error[key]);
}
return result;
}
return error;
};

// TODO this is really over complicated now
// Because we're taking closer control of errors
// we should be able to report more simply
const createErrorReporter = (logger: Logger): ErrorReporter => {
return (state, stepId, error) => {
// TODO I don't think the report is useful anymore
// we'll strip it all out soon
// see https://github.com/OpenFn/kit/issues/726
const report: ErrorReport = {
type: error.subtype || error.type || error.name,
stepId,
Expand All @@ -34,15 +49,7 @@ const createErrorReporter = (logger: Logger): ErrorReporter => {
logger.error('CRITICAL ERROR! Aborting execution');
}

if (report.message) {
logger.error(
`${report.code || report.type || 'Error'}: ${report.message}`
);
logger.debug(error); // TODO the logger doesn't handle this very well
} else {
// This catches if a non-Error object is thrown, ie, `throw "e"`
logger.error(error);
}
logger.error(error);

if (error.severity === 'fail') {
logger.error(`Check state.errors.${stepId} for details.`);
Expand All @@ -51,7 +58,7 @@ const createErrorReporter = (logger: Logger): ErrorReporter => {
state.errors = {};
}

state.errors[stepId] = report;
state.errors[stepId] = serialize(error);
}

return report as ErrorReport;
Expand Down
4 changes: 3 additions & 1 deletion packages/runtime/test/__modules__/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ export const x = 'test';
export default x;

export const err = () => {
throw new Error('adaptor err');
const e = new Error('adaptor err');
e.code = 1234;
throw e;
};

export const err2 = () => {
Expand Down
120 changes: 79 additions & 41 deletions packages/runtime/test/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ test('crash on timeout', async (t) => {
const expression = 'export default [(s) => new Promise((resolve) => {})]';

const plan = createPlan(expression, { timeout: 1 });
let error;
let error: any;
try {
await run(plan);
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'kill');
t.is(error.type, 'TimeoutError');
t.is(error.message, 'Job took longer than 1ms to complete');
Expand All @@ -35,14 +35,14 @@ test('crash on timeout', async (t) => {
test('crash on runtime error with SyntaxError', async (t) => {
const expression = 'export default [(s) => ~@]2q1j]';

let error;
let error: any;
try {
await run(expression);
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'crash');
t.is(error.type, 'RuntimeCrash');
t.is(error.subtype, 'SyntaxError');
Expand All @@ -52,12 +52,13 @@ test('crash on runtime error with SyntaxError', async (t) => {
test('crash on runtime error with ReferenceError', async (t) => {
const expression = 'export default [(s) => x]';

let error;
let error: any;
try {
await run(expression);
} catch (e) {
error = e;
}
t.log(error);

// t.true(error instanceof RuntimeError);
t.is(error.severity, 'crash');
Expand All @@ -69,14 +70,14 @@ test('crash on runtime error with ReferenceError', async (t) => {
test('crash on eval with SecurityError', async (t) => {
const expression = 'export default [(s) => eval("process.exit()")]';

let error;
let error: any;
try {
await run(expression);
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'kill');
t.is(error.type, 'SecurityError');
t.is(error.message, 'Illegal eval statement detected');
Expand All @@ -101,14 +102,14 @@ test('crash on edge condition error with EdgeConditionError', async (t) => {
},
};

let error;
let error: any;
try {
await run(plan);
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'crash');
t.is(error.type, 'EdgeConditionError');
t.is(error.message, 'wibble is not defined');
Expand All @@ -122,14 +123,14 @@ test.todo('crash on input error if a function is passed with forceSandbox');
test('crash on import error: module path provided', async (t) => {
const expression = 'import x from "blah"; export default [(s) => x]';

let error;
let error: any;
try {
await run(expression);
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'crash');
t.is(error.type, 'ImportError');
t.is(error.message, 'Failed to import module "blah"');
Expand All @@ -138,7 +139,7 @@ test('crash on import error: module path provided', async (t) => {
test('crash on blacklisted module', async (t) => {
const expression = 'import x from "blah"; export default [(s) => x]';

let error;
let error: any;
try {
await run(
expression,
Expand All @@ -152,8 +153,8 @@ test('crash on blacklisted module', async (t) => {
} catch (e) {
error = e;
}
t.log(error);

t.truthy(error);
t.is(error.severity, 'crash');
t.is(error.type, 'ImportError');
t.is(error.message, 'module blacklisted: blah');
Expand All @@ -162,61 +163,80 @@ test('crash on blacklisted module', async (t) => {
test('fail on runtime TypeError', async (t) => {
const expression = 'export default [(s) => s.x.y]';

const result = await run(expression);
const result: any = await run(expression);
const error = result.errors['job-1'];

t.truthy(error);
t.is(error.type, 'TypeError');
t.is(
error.message,
"TypeError: Cannot read properties of undefined (reading 'y')"
);
t.log(error);

t.deepEqual(error, {
message: "TypeError: Cannot read properties of undefined (reading 'y')",
name: 'RuntimeError',
type: 'RuntimeError',
subtype: 'TypeError',
severity: 'fail',
source: 'runtime',
});
});

// TODO not totally convinced on this one actually
test('fail on runtime error with RangeError', async (t) => {
const expression =
'export default [(s) => Number.parseFloat("1").toFixed(-1)]';

const result = await run(expression);
const result: any = await run(expression);
const error = result.errors['job-1'];

t.truthy(error);
t.is(error.type, 'RangeError');
t.is(
error.message,
'RangeError: toFixed() digits argument must be between 0 and 100'
);
t.log(error);

t.deepEqual(error, {
message: 'RangeError: toFixed() digits argument must be between 0 and 100',
name: 'RuntimeError',
subtype: 'RangeError',
type: 'RuntimeError',
severity: 'fail',
source: 'runtime',
});
});

test('fail on user error with new Error()', async (t) => {
const expression = 'export default [(s) => {throw new Error("abort")}]';

const result = await run(expression);
const result: any = await run(expression);

const error = result.errors['job-1'];

t.is(error.type, 'JobError');
t.is(error.message, 'abort');
t.log(error);

t.deepEqual(error, {
message: 'abort',
name: 'JobError',
severity: 'fail',
source: 'runtime',
type: 'JobError',
});
});

test('fail on user error with throw "abort"', async (t) => {
const expression = 'export default [(s) => {throw "abort"}]';

const result = await run(expression);
const result: any = await run(expression);

const error = result.errors['job-1'];

t.is(error.type, 'JobError');
t.is(error.message, 'abort');
t.log(error);

t.deepEqual(error, {
message: 'abort',
name: 'JobError',
severity: 'fail',
source: 'runtime',
type: 'JobError',
});
});

test('fail on adaptor error (with throw new Error())', async (t) => {
const expression = `
import { err } from 'x';
export default [(s) => err()];
`;
const result = await run(
const result: any = await run(
expression,
{},
{
Expand All @@ -229,8 +249,18 @@ test('fail on adaptor error (with throw new Error())', async (t) => {
);

const error = result.errors['job-1'];
t.is(error.type, 'AdaptorError');
t.is(error.message, 'adaptor err');
t.log(error);

t.deepEqual(error, {
details: {
code: 1234,
},
message: 'adaptor err',
name: 'AdaptorError',
type: 'AdaptorError',
source: 'runtime',
severity: 'fail',
});
});

test('adaptor error with no stack trace will be a user error', async (t) => {
Expand All @@ -254,6 +284,14 @@ test('adaptor error with no stack trace will be a user error', async (t) => {
);

const error = result.errors['job-1'];
t.is(error.type, 'JobError');
t.is(error.message, 'adaptor err');

t.log(error);

t.deepEqual(error, {
message: 'adaptor err',
name: 'JobError',
severity: 'fail',
source: 'runtime',
type: 'JobError',
});
});
Loading