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(cli): improve error logging #2071

Merged
merged 5 commits into from
Mar 3, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 4 additions & 2 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@

{
"files": [
"src/**/__tests__/**/*.jest.test.{ts,js}"
"src/**/__tests__/**/*.jest.test.{ts,js}",
"__mocks__/**/*.{ts,js}"
],
"env": {
"jest": true
"jest": true,
"node": true
}
},

Expand Down
1 change: 0 additions & 1 deletion __karma__/process.js

This file was deleted.

7 changes: 0 additions & 7 deletions __mocks__/nanoid/non-secure.ts

This file was deleted.

15 changes: 15 additions & 0 deletions __mocks__/process.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module.exports.exit = jest.fn();
module.exports.cwd = jest.fn();
module.exports.on = jest.fn();

module.exports.stdin = {
fd: 0,
isTTY: true,
};
module.exports.stdout = {
write: jest.fn(),
};

module.exports.stderr = {
write: jest.fn(),
};
2 changes: 1 addition & 1 deletion karma.conf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ module.exports = (config: Config): void => {
'nimma/legacy': require.resolve('./node_modules/nimma/dist/legacy/cjs/index.js'),
'node-fetch': require.resolve('./__karma__/fetch'),
fs: require.resolve('./__karma__/fs'),
process: require.resolve('./__karma__/process'),
process: require.resolve('./__mocks__/process'),
perf_hooks: require.resolve('./__karma__/perf_hooks'),
fsevents: require.resolve('./__karma__/fsevents'),
},
Expand Down
7 changes: 6 additions & 1 deletion packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,18 @@
"lodash": "~4.17.21",
"pony-cause": "^1.0.0",
"proxy-agent": "5.0.0",
"stacktracey": "^2.1.7",
"strip-ansi": "6.0",
"text-table": "0.2",
"tslib": "^2.3.0",
"yargs": "17.3.1"
},
"devDependencies": {
"@types/es-aggregate-error": "^1.0.2",
"@types/xml2js": "^0.4.9",
"@types/yargs": "^17.0.8",
"copyfiles": "^2.4.1",
"es-aggregate-error": "^1.0.7",
"jest-when": "^3.4.2",
"nock": "^13.1.3",
"node-html-parser": "^4.1.5",
Expand All @@ -73,7 +76,9 @@
],
"assets": [
"./dist/**/*.json",
"./dist/**/*.html"
"./dist/**/*.html",
"../*/dist/**/*.js.map",
"../*/src/**/*.ts"
]
}
}
81 changes: 58 additions & 23 deletions packages/cli/src/commands/__tests__/lint.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import * as yargs from 'yargs';
import { noop } from 'lodash';
import { DiagnosticSeverity } from '@stoplight/types';
import { IRuleResult } from '@stoplight/spectral-core';
import * as process from 'process';
import { ErrorWithCause } from 'pony-cause';
import AggregateError from 'es-aggregate-error';

import { lint } from '../../services/linter';
import { formatOutput, writeOutput } from '../../services/output';
import lintCommand from '../lint';
import chalk from 'chalk';

jest.mock('process');
jest.mock('../../services/output');
jest.mock('../../services/linter');

Expand All @@ -24,9 +28,6 @@ function run(command: string) {
}

describe('lint', () => {
let errorSpy: jest.SpyInstance;
const { isTTY } = process.stdin;

const results: IRuleResult[] = [
{
code: 'parser',
Expand All @@ -47,29 +48,26 @@ describe('lint', () => {
];

beforeEach(() => {
(lint as jest.Mock).mockClear();
(lint as jest.Mock).mockResolvedValueOnce(results);

(formatOutput as jest.Mock).mockClear();
(formatOutput as jest.Mock).mockReturnValueOnce('<formatted output>');

(writeOutput as jest.Mock).mockClear();
(writeOutput as jest.Mock).mockResolvedValueOnce(undefined);

errorSpy = jest.spyOn(console, 'error').mockImplementation(noop);
});

afterEach(() => {
errorSpy.mockRestore();
process.stdin.isTTY = isTTY;
process.stdin.isTTY = true;
jest.clearAllMocks();
jest.resetAllMocks();
});

it('shows help when no document and no STDIN are present', () => {
process.stdin.isTTY = true;
return expect(run('lint')).rejects.toContain('documents Location of JSON/YAML documents');
});

describe('when STDIN is present', () => {
beforeEach(() => {
process.stdin.isTTY = false;
});

it('does not show help when documents are missing', async () => {
const output = await run('lint');
expect(output).not.toContain('documents Location of JSON/YAML documents');
Expand Down Expand Up @@ -150,35 +148,29 @@ describe('lint', () => {

it.each(['json', 'stylish'])('calls formatOutput with %s format', async format => {
await run(`lint -f ${format} ./__fixtures__/empty-oas2-document.json`);
await new Promise(resolve => void process.nextTick(resolve));
expect(formatOutput).toBeCalledWith(results, format, { failSeverity: DiagnosticSeverity.Error });
});

it('writes formatted output to a file', async () => {
await run(`lint -o foo.json ./__fixtures__/empty-oas2-document.json`);
await new Promise(resolve => void process.nextTick(resolve));
expect(writeOutput).toBeCalledWith('<formatted output>', 'foo.json');
});

it('writes formatted output to multiple files when using format and output flags', async () => {
(formatOutput as jest.Mock).mockClear();
(formatOutput as jest.Mock).mockReturnValue('<formatted output>');

await run(
`lint --format html --format json --output.json foo.json --output.html foo.html ./__fixtures__/empty-oas2-document.json`,
);
await new Promise(resolve => void process.nextTick(resolve));
expect(writeOutput).toBeCalledTimes(2);
expect(writeOutput).nthCalledWith(1, '<formatted output>', 'foo.html');
expect(writeOutput).nthCalledWith(2, '<formatted output>', 'foo.json');
});

it('writes formatted output to multiple files and stdout when using format and output flags', async () => {
(formatOutput as jest.Mock).mockClear();
(formatOutput as jest.Mock).mockReturnValue('<formatted output>');

await run(`lint --format html --format json --output.json foo.json ./__fixtures__/empty-oas2-document.json`);
await new Promise(resolve => void process.nextTick(resolve));
expect(writeOutput).toBeCalledTimes(2);
expect(writeOutput).nthCalledWith(1, '<formatted output>', '<stdout>');
expect(writeOutput).nthCalledWith(2, '<formatted output>', 'foo.json');
Expand Down Expand Up @@ -216,8 +208,51 @@ describe('lint', () => {
const error = new Error('Failure');
(lint as jest.Mock).mockReset();
(lint as jest.Mock).mockRejectedValueOnce(error);
await run(`lint -o foo.json ./__fixtures__/empty-oas2-document.json`);
await new Promise(resolve => void process.nextTick(resolve));
expect(errorSpy).toBeCalledWith('Failure');
await run(`lint ./__fixtures__/empty-oas2-document.json`);
expect(process.stderr.write).nthCalledWith(1, chalk.red('Error running Spectral!\n'));
expect(process.stderr.write).nthCalledWith(2, chalk.red('Use --verbose flag to print the error stack.\n'));
expect(process.stderr.write).nthCalledWith(3, `Error #1: ${chalk.red('Failure')}\n`);
});

it('prints each error separately', async () => {
(lint as jest.Mock).mockReset();
(lint as jest.Mock).mockRejectedValueOnce(
new AggregateError([
new Error('some unhandled exception'),
new TypeError('another one'),
new ErrorWithCause('some error with cause', { cause: 'original exception' }),
]),
);
await run(`lint ./__fixtures__/empty-oas2-document.json`);
expect(process.stderr.write).nthCalledWith(3, `Error #1: ${chalk.red('some unhandled exception')}\n`);
expect(process.stderr.write).nthCalledWith(4, `Error #2: ${chalk.red('another one')}\n`);
expect(process.stderr.write).nthCalledWith(5, `Error #3: ${chalk.red('original exception')}\n`);
});

it('given verbose flag, prints each error together with their stacks', async () => {
(lint as jest.Mock).mockReset();
(lint as jest.Mock).mockRejectedValueOnce(
new AggregateError([
new Error('some unhandled exception'),
new TypeError('another one'),
new ErrorWithCause('some error with cause', { cause: 'original exception' }),
]),
);

await run(`lint --verbose ./__fixtures__/empty-oas2-document.json`);

expect(process.stderr.write).nthCalledWith(2, `Error #1: ${chalk.red('some unhandled exception')}\n`);
expect(process.stderr.write).nthCalledWith(
3,
expect.stringContaining(`packages/cli/src/commands/__tests__/lint.test.ts:236`),
);

expect(process.stderr.write).nthCalledWith(4, `Error #2: ${chalk.red('another one')}\n`);
expect(process.stderr.write).nthCalledWith(
5,
expect.stringContaining(`packages/cli/src/commands/__tests__/lint.test.ts:237`),
);

expect(process.stderr.write).nthCalledWith(6, `Error #3: ${chalk.red('original exception')}\n`);
});
});
Loading