Skip to content

Commit

Permalink
fix: oclif-friendly error logging (#1080)
Browse files Browse the repository at this point in the history
| 🚥 Resolves RM-11449 |
| :------------------- |

## 🧰 Changes

Fixes up the error logging so it maintains parity with `rdme@8`, while
doing so in a way that's friendly with oclif.

Also slightly reworked our vitest config to load env variables a little
bit more smoothly.

## 🧬 QA & Testing

Do tests still pass?
  • Loading branch information
kanadgupta authored Nov 21, 2024
1 parent 6439425 commit 73c5f4c
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 40 deletions.
9 changes: 4 additions & 5 deletions __tests__/commands/docs/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import fs from 'node:fs';
import path from 'node:path';

import { runCommand as oclifRunCommand } from '@oclif/test';
import chalk from 'chalk';
import frontMatter from 'gray-matter';
import nock from 'nock';
Expand All @@ -16,7 +15,7 @@ import { getAPIV1Mock, getAPIV1MockWithVersionHeader } from '../../helpers/get-a
import { after, before } from '../../helpers/get-gha-setup.js';
import hashFileContents from '../../helpers/hash-file-contents.js';
import { after as afterGHAEnv, before as beforeGHAEnv } from '../../helpers/setup-gha-env.js';
import { runCommand } from '../../helpers/setup-oclif-config.js';
import { runCommand, runCommandWithHooks } from '../../helpers/setup-oclif-config.js';

const fixturesBaseDir = '__fixtures__/docs';
const fullFixturesDir = `${__dirname}./../../${fixturesBaseDir}`;
Expand Down Expand Up @@ -722,9 +721,9 @@ describe('rdme docs', () => {

describe('rdme guides', () => {
it('should error if no path provided', async () => {
return expect((await oclifRunCommand(['guides', '--key', key, '--version', '1.0.0'])).error.message).toContain(
'Missing 1 required arg:\npath',
);
return expect(
(await runCommandWithHooks(['guides', '--key', key, '--version', '1.0.0'])).error.message,
).toContain('Missing 1 required arg:\npath');
});
});
});
5 changes: 2 additions & 3 deletions __tests__/commands/docs/prune.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import { runCommand as oclifRunCommand } from '@oclif/test';
import nock from 'nock';
import prompts from 'prompts';
import { describe, beforeAll, afterAll, it, expect } from 'vitest';

import Command from '../../../src/commands/docs/prune.js';
import { getAPIV1Mock, getAPIV1MockWithVersionHeader } from '../../helpers/get-api-mock.js';
import { runCommand } from '../../helpers/setup-oclif-config.js';
import { runCommand, runCommandWithHooks } from '../../helpers/setup-oclif-config.js';

const fixturesBaseDir = '__fixtures__/docs';

Expand Down Expand Up @@ -166,7 +165,7 @@ describe('rdme docs:prune', () => {
describe('rdme guides:prune', () => {
it('should error if no folder provided', async () => {
return expect(
(await oclifRunCommand(['guides:prune', '--key', key, '--version', version])).error.message,
(await runCommandWithHooks(['guides:prune', '--key', key, '--version', version])).error.message,
).toContain('Missing 1 required arg:\nfolder');
});
});
Expand Down
5 changes: 2 additions & 3 deletions __tests__/commands/openapi/validate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@

import fs from 'node:fs';

import { runCommand as oclifRunCommand } from '@oclif/test';
import chalk from 'chalk';
import prompts from 'prompts';
import { describe, beforeAll, beforeEach, afterEach, it, expect, vi, type MockInstance } from 'vitest';

import Command from '../../../src/commands/openapi/validate.js';
import { after, before } from '../../helpers/get-gha-setup.js';
import { runCommand } from '../../helpers/setup-oclif-config.js';
import { runCommand, runCommandWithHooks } from '../../helpers/setup-oclif-config.js';

let consoleInfoSpy: MockInstance;

Expand Down Expand Up @@ -122,7 +121,7 @@ describe('rdme openapi:validate', () => {
it('should fail if user attempts to pass `--github` flag in CI environment', async () => {
return expect(
(
await oclifRunCommand([
await runCommandWithHooks([
'openapi:validate',
'__tests__/__fixtures__/petstore-simple-weird-version.json',
'--github',
Expand Down
27 changes: 18 additions & 9 deletions __tests__/helpers/setup-oclif-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import type { Command as OclifCommand } from '@oclif/core';
import path from 'node:path';

import { Config } from '@oclif/core';
import { captureOutput } from '@oclif/test';
import { captureOutput, runCommand as oclifRunCommand } from '@oclif/test';

const testNodeEnv = process.env.NODE_ENV;

/**
* Used for setting up the oclif configuration for simulating commands in tests.
Expand All @@ -28,13 +30,20 @@ export function runCommand<T extends typeof OclifCommand>(Command: T) {
const oclifConfig = await setupOclifConfig();
// @ts-expect-error this is the pattern recommended by the @oclif/test docs.
// Not sure how to get this working with type generics.
return captureOutput<string>(() => Command.run(args, oclifConfig), { testNodeEnv: 'rdme-test' }).then(
({ error, result }) => {
if (error) {
throw error;
}
return result;
},
);
return captureOutput<string>(() => Command.run(args, oclifConfig), { testNodeEnv }).then(({ error, result }) => {
if (error) {
throw error;
}
return result;
});
};
}

/**
* A lightweight wrapper around `@oclif/test`'s `runCommand`
* that loads our mock config and mock test env.
*/
export async function runCommandWithHooks(args?: string[]) {
const oclifConfig = await setupOclifConfig();
return oclifRunCommand(args, oclifConfig, { testNodeEnv });
}
15 changes: 0 additions & 15 deletions __tests__/setup.js

This file was deleted.

39 changes: 35 additions & 4 deletions src/lib/baseCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { format } from 'node:util';

import * as core from '@actions/core';
import { Command as OclifCommand } from '@oclif/core';
import chalk from 'chalk';
import debugPkg from 'debug';

import { isGHA, isTest } from './isCI.js';
Expand Down Expand Up @@ -44,10 +45,40 @@ export default abstract class BaseCommand<T extends typeof OclifCommand> extends

abstract run(): Promise<string>;

protected async catch(err: Error & { exitCode?: number }): Promise<unknown> {
// add any custom logic to handle errors from the command
// or simply return the parent class error handling
return super.catch(err);
protected async catch(err: Error & { exitCode?: number }) {
if (isTest()) {
return super.catch(err);
}

let message = `Yikes, something went wrong! Please try again and if the problem persists, get in touch with our support team at ${chalk.underline(
'[email protected]',
)}.`;

if (err.message) {
message = err.message;
}

/**
* If we're in a GitHub Actions environment, log errors with that formatting instead.
*
* @see {@link https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message}
* @see {@link https://github.com/actions/toolkit/tree/main/packages/core#annotations}
*/
if (isGHA()) {
return core.setFailed(message);
}

// If this is a soft error then we should output the result as a regular log but exit the CLI
// with an error status code.
if (err.name === 'SoftError') {
// eslint-disable-next-line no-console
console.log(err.message);
} else {
// eslint-disable-next-line no-console
console.error(chalk.red(`\n${message}\n`));
}

return process.exit(process.exitCode ?? err.exitCode ?? 1);
}

/**
Expand Down
15 changes: 14 additions & 1 deletion vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,27 @@ export default defineConfig({
coverage: {
exclude: [...coverageConfigDefaults.exclude, '**/dist-gha/**'],
},
env: {
/**
* The `chalk` and `colors` libraries have trouble with tests sometimes in test snapshots so
* we're disabling colorization here for all tests.
*
* @see {@link https://github.com/chalk/supports-color/issues/106}
*/
FORCE_COLOR: '0',
/**
* Sets our test `NODE_ENV` to a custom value in case of false positives if someone is using this
* tool in a testing environment.
*/
NODE_ENV: 'rdme-test',
},
exclude: [
'**/__fixtures__/**',
'**/dist-gha/**',
'**/helpers/**',
'**/__snapshots__/**',
...configDefaults.exclude,
],
globalSetup: ['./__tests__/setup.js'],
onConsoleLog(log: string, type: 'stderr' | 'stdout'): boolean | void {
// hides `rdme open` deprecation warning
return !(log.includes('`rdme open` is deprecated and will be removed in a future release') && type === 'stderr');
Expand Down

0 comments on commit 73c5f4c

Please sign in to comment.