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: control fail severity and result display #540

Merged
merged 4 commits into from
Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
29 changes: 21 additions & 8 deletions docs/guides/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,29 @@ spectral lint petstore.yaml
Other options include:

``` text
-e, --encoding=encoding text encoding to use
-f, --format=json|stylish formatter to use for outputting results
-h, --help show CLI help
-o, --output=output output to a file instead of stdout
-q, --quiet no logging - output only
-r, --ruleset=ruleset path to a ruleset file (supports remote files)
-s, --skip-rule=skip-rule ignore certain rules if they are causing trouble
-v, --verbose increase verbosity
--version Show version number [boolean]
--help Show help [boolean]
--encoding, -e text encoding to use [string] [default: "utf8"]
--format, -f formatter to use for outputting results [string] [default: "stylish"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the lack of available formats expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not no. damn where'd they go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

This PR did not remove them, the test harness shows that develop branch is missing them already. Let's carry on with this merge and fix them in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alrighty, sounds good.

--output, -o output to a file instead of stdout [string]
--ruleset, -r path/URL to a ruleset file [string]
--skip-rule, -s ignore certain rules if they are causing trouble [string]
--fail-severity, -F results of this level or above will trigger a failure exit code
[string] [choices: "error", "warn", "info", "hint"] [default: "hint"]
--display-only-failures, -D only output results equal to or greater than --fail-severity
Copy link
Contributor

Choose a reason for hiding this comment

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

greater sounds a bit unfortunate, given DiagnosticSeverity.Error equals 0, Warn 1 etc.
I know what the intention is, but perhaps there is a way to make it more clear, i.e. more severe or higher/lower?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

greater and higher are synonomous and i dont think anyone knows what the internal ints are? This is probably find, if anyone gets confused we can ask them for suggestions.

[boolean] [default: false]
--verbose, -v increase verbosity [boolean]
--quiet, -q no logging - output only [boolean]
```

> Note: The Spectral CLI supports both YAML and JSON.

Currently, Spectral CLI supports validation of OpenAPI v2/v3 documents via our built-in ruleset, or you can create [custom rulesets](../getting-started/rulesets.md) to work with any JSON/YAML documents.

## Error Results

Spectral has a few different error severities: `error`, `warn`, `info` and `hint`, and they are in "order" from highest to lowest. By default, all results will be shown regardless of severity, and the presence of any results will cause a failure status code of 1.

The default behavior is can be modified with the `--fail-severity=` option. Setting fail severity to `--fail-severity=warn` would return a status code of 1 for any warning results or higher, so that would also include error. Using `--fail-severity=error` will only show errors.

Changing the fail severity will not effect output. To change what results Spectral CLI prints to the screen, add the `--display-only-failures` switch (or just `-D` for short). This will strip out any results which are below the fail severity.
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
"build.binary": "pkg . --targets linux,macos --out-path ./binaries",
"build.oas-functions": "rollup -c",
"build": "tsc -p ./tsconfig.build.json",
"cli": "node -r ts-node/register -r tsconfig-paths/register src/cli/index.ts",
"cli:debug": "node -r ts-node/register -r tsconfig-paths/register --inspect-brk src/cli/index.ts",
"compile-rulesets": "node ./scripts/compile-rulesets.js",
"inline-version": "./scripts/inline-version.js",
"lint.fix": "yarn lint --fix",
Expand Down
54 changes: 48 additions & 6 deletions src/cli/commands/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { Dictionary } from '@stoplight/types';
import { CommandModule, showHelp } from 'yargs';

import { pick } from 'lodash';
import { ILintConfig, OutputFormat } from '../../types/config';
import { getDiagnosticSeverity } from '../../rulesets/severity';
import { IRuleResult } from '../../types';
import { FailSeverity, ILintConfig, OutputFormat } from '../../types/config';
import { lint } from '../services/linter';
import { formatOutput, writeOutput } from '../services/output';

Expand Down Expand Up @@ -58,6 +60,19 @@ const lintCommand: CommandModule = {
type: 'string',
coerce: toArray,
},
'fail-severity': {
alias: 'F',
description: 'results of this level or above will trigger a failure exit code',
choices: ['error', 'warn', 'info', 'hint'],
default: 'hint', // TODO: BREAKING: raise this to warn in 5.0
type: 'string',
},
'display-only-failures': {
alias: 'D',
description: 'only output results equal to or greater than --fail-severity',
Copy link
Contributor

Choose a reason for hiding this comment

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

As in docs.

type: 'boolean',
default: false,
},
verbose: {
alias: 'v',
description: 'increase verbosity',
Expand All @@ -71,20 +86,37 @@ const lintCommand: CommandModule = {
}),

handler: args => {
const { document, ruleset, format, output, encoding, ...config } = (args as unknown) as ILintConfig & {
const {
document,
failSeverity,
displayOnlyFailures,
ruleset,
format,
output,
encoding,
...config
} = (args as unknown) as ILintConfig & {
document: string;
failSeverity: FailSeverity;
displayOnlyFailures: boolean;
};

return lint(
document,
{ format, output, encoding, ...pick(config, ['ruleset', 'skipRule', 'verbose', 'quiet']) },
ruleset,
)
.then(results => {
if (displayOnlyFailures) {
return filterResultsBySeverity(results, failSeverity);
}
return results;
})
.then(results => {
if (results.length) {
process.exitCode = 1;
process.exitCode = severeEnoughToFail(results, failSeverity) ? 1 : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
process.exitCode = severeEnoughToFail(results, failSeverity) ? 1 : 0;
process.exitCode = isSevereEnoughToFail(results, failSeverity) ? 1 : 0;

} else if (!config.quiet) {
console.log('No errors or warnings found!');
console.log(`No results with a severity of '${failSeverity}' or higher found!`);
}
const formattedOutput = formatOutput(results, format);
return writeOutput(formattedOutput, output);
Expand All @@ -93,9 +125,19 @@ const lintCommand: CommandModule = {
},
};

function fail(err: Error) {
const fail = (err: Error) => {
console.error(err);
process.exitCode = 2;
}
};

const filterResultsBySeverity = (results: IRuleResult[], failSeverity: FailSeverity): IRuleResult[] => {
const diagnosticSeverity = getDiagnosticSeverity(failSeverity);
return results.filter(r => r.severity <= diagnosticSeverity);
};

const severeEnoughToFail = (results: IRuleResult[], failSeverity: FailSeverity): boolean => {
const diagnosticSeverity = getDiagnosticSeverity(failSeverity);
return results.some(r => r.severity <= diagnosticSeverity);
};

export default lintCommand;
2 changes: 1 addition & 1 deletion src/fs/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export interface IReadOptions {
export async function readFile(name: string, opts: IReadOptions) {
if (isURL(name)) {
let response;
let timeout: NodeJS.Timeout | null = null;
let timeout: NodeJS.Timeout | number | null = null;
try {
if (opts.timeout) {
const controller = new AbortController();
Expand Down
2 changes: 1 addition & 1 deletion src/rulesets/mergers/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function mergeFunctions(
if (typeof rule === 'object') {
const ruleThen = Array.isArray(rule.then) ? rule.then : [rule.then];
for (const then of ruleThen) {
// note: if function relies on global function, it will take the most recent defined one
// if function relies on global function, it will take the most recently defined one
if (then.function in map) {
then.function = map[then.function];
}
Expand Down
16 changes: 16 additions & 0 deletions src/rulesets/oas/functions/__tests__/oasOp2xxResponse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,20 @@ describe('oasOp2xxResponse', () => {
},
]);
});

test('does not complain when no $.responses property', async () => {
const results = await s.run({
paths: {
'/test': {
get: {
operationId: '123',
},
post: {
operationId: '123',
},
},
},
});
expect(results).toEqual([]);
});
});
7 changes: 4 additions & 3 deletions src/rulesets/oas/functions/oasOp2xxResponse.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import { IFunction, IFunctionResult, Rule } from '../../../types';

export const oasOp2xxResponse: IFunction<Rule> = targetVal => {
const results: IFunctionResult[] = [];
if (!targetVal) {
return;
}

const results: IFunctionResult[] = [];
const responses = Object.keys(targetVal);

if (responses.filter(response => Number(response) >= 200 && Number(response) < 300).length === 0) {
results.push({
message: 'operations must define at least one 2xx response',
});
}

return results;
};

Expand Down
4 changes: 4 additions & 0 deletions src/types/config.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import { HumanReadableDiagnosticSeverity } from './rule';

export type FailSeverity = HumanReadableDiagnosticSeverity;

export enum OutputFormat {
JSON = 'json',
STYLISH = 'stylish',
Expand Down
4 changes: 3 additions & 1 deletion test-harness/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
export function parseScenarioFile(data: string) {
const regex = /====(test|document|command|stdout|stderr)====\r?\n/gi;
const regex = /====(test|document|command|status|stdout|stderr)====\r?\n/gi;
const split = data.split(regex);

const testIndex = split.findIndex(t => t === 'test');
const documentIndex = split.findIndex(t => t === 'document');
const commandIndex = split.findIndex(t => t === 'command');
const statusIndex = split.findIndex(t => t === 'status');
const stdoutIndex = split.findIndex(t => t === 'stdout');
const stderrIndex = split.findIndex(t => t === 'stderr');

return {
test: split[1 + testIndex],
document: split[1 + documentIndex],
command: split[1 + commandIndex],
status: split[1 + statusIndex],
stdout: split[1 + stdoutIndex],
stderr: split[1 + stderrIndex],
};
Expand Down
21 changes: 13 additions & 8 deletions test-harness/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ function replaceVars(string: string, replacements: Replacement[]) {
return replacements.reduce((str, replace) => str.replace(replace.from, replace.to), string);
}

describe('cli e2e tests', () => {
const files = process.env.TESTS
? String(process.env.TESTS).split(',')
: glob.readdirSync('**/*.scenario', { cwd: path.join(__dirname, './scenarios') });
describe('cli acceptance tests', () => {
const cwd = path.join(__dirname, './scenarios');
const files = process.env.TESTS ? String(process.env.TESTS).split(',') : glob.readdirSync('**/*.scenario', { cwd });

files.forEach((file: string) => {
const data = fs.readFileSync(path.join(__dirname, './scenarios/', file), { encoding: 'utf8' });
const data = fs.readFileSync(path.join(cwd, file), { encoding: 'utf8' });
const scenario = parseScenarioFile(data);
const replacements: Replacement[] = [];

Expand Down Expand Up @@ -60,7 +59,7 @@ describe('cli e2e tests', () => {
}
});

test(`${file}${os.EOL}${scenario.test}`, () => {
test(`./test-harness/scenarios/${file}${os.EOL}${scenario.test}`, () => {
// TODO split on " " is going to break quoted args
const args = scenario.command.split(' ').map(t => {
const arg = t.trim();
Expand All @@ -76,10 +75,12 @@ describe('cli e2e tests', () => {
windowsVerbatimArguments: false,
});

const expectedStatus = replaceVars(scenario.status.trim(), replacements);
const expectedStdout = replaceVars(scenario.stdout.trim(), replacements);
const expectedStderr = replaceVars(scenario.stderr.trim(), replacements);
const status = commandHandle.status;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's maybe differentiate between expected and actual to be consistent? here, actualStatus would tell more than just status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actualStatus is the status. I think t's ok.

const stderr = commandHandle.stderr.trim();
const stdout = commandHandle.stdout.trim();
const expectedStderr = replaceVars(scenario.stderr.trim(), replacements);
const expectedStdout = replaceVars(scenario.stdout.trim(), replacements);

if (expectedStderr) {
expect(stderr).toEqual(expectedStderr);
Expand All @@ -90,6 +91,10 @@ describe('cli e2e tests', () => {
if (stdout) {
expect(stdout).toEqual(expectedStdout);
}

if (expectedStatus !== '') {
expect(`status:${status}`).toEqual(`status:${expectedStatus}`);
}
});
});
});
20 changes: 11 additions & 9 deletions test-harness/scenarios/help-no-document.scenario
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ Positionals:
document Location of a JSON/YAML document. Can be either a file or a fetchable resource on the web. [string] [required]

Options:
--version Show version number [boolean]
--help Show help [boolean]
--encoding, -e text encoding to use [string] [default: "utf8"]
--format, -f formatter to use for outputting results [string] [default: "stylish"]
--output, -o output to a file instead of stdout [string]
--ruleset, -r path/URL to a ruleset file [string]
--skip-rule, -s ignore certain rules if they are causing trouble [string]
--verbose, -v increase verbosity [boolean]
--quiet, -q no logging - output only [boolean]
--version Show version number [boolean]
--help Show help [boolean]
--encoding, -e text encoding to use [string] [default: "utf8"]
--format, -f formatter to use for outputting results [string] [default: "stylish"]
--output, -o output to a file instead of stdout [string]
--ruleset, -r path/URL to a ruleset file [string]
--skip-rule, -s ignore certain rules if they are causing trouble [string]
--fail-severity, -F results of this level or above will trigger a failure exit code [string] [choices: "error", "warn", "info", "hint"] [default: "hint"]
--display-only-failures, -D only output results equal to or greater than --fail-severity [boolean] [default: false]
--verbose, -v increase verbosity [boolean]
--quiet, -q no logging - output only [boolean]
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@ components:
lint --ruleset ./test-harness/scenarios/rulesets/parameter-description.oas3.yaml {document}
====stdout====
OpenAPI 3.x detected
No errors or warnings found!
No results with a severity of 'hint' or higher found!
47 changes: 47 additions & 0 deletions test-harness/scenarios/severity/display-errors.oas3.scenario
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
====test====
Request only errors be shown, but no errors exist
====document====
openapi: '3.0.0'
info:
version: 1.0.0
title: Swagger Petstore
license:
name: MIT
paths:
/pets/{petId}:
get:
summary: Info for a specific pet
operationId: showPetById
tags:
- pets
parameters:
- name: petId
in: path
required: true
description: The id of the pet to retrieve
schema:
type: string
responses:
'200':
description: Expected response to a valid request
content:
application/json:
schema:
required:
- id
- name
properties:
id:
type: integer
format: int64
name:
type: string
tag:
type: string
====command====
lint {document} --fail-severity=error -D
====status====
0
====stdout====
OpenAPI 3.x detected
No results with a severity of 'error' or higher found!
Loading