From 85666c5a2a221f86f3315778ceb7e0959147c91b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Mond=C3=A9jar=20Rubio?= Date: Fri, 7 Jun 2024 17:23:13 +0200 Subject: [PATCH] fix: Fix race condition when using the API (#107) * fix: Fix race condition when using the API BREAKING CHANGE: Users consuming the API must call now `Linting().lint()` --- .github/workflows/test.yml | 1 + README.md | 2 ++ bin/cli.js | 2 ++ src/lib/linting.js | 2 -- test/api.spec.js | 38 +++++++++++++++++++++++++------------- test/cli.spec.js | 4 ++-- test/helpers.js | 32 ++++---------------------------- test/reporter.spec.js | 1 + 8 files changed, 37 insertions(+), 45 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 02b7b2f..652c42c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,6 +33,7 @@ jobs: - 16 - 18 - 20 + - 22 include: - os: macos-latest node-version: 20 diff --git a/README.md b/README.md index 78f5c0e..bcb4de3 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,8 @@ linting.on("done", () => { console.log("You've been a naughty boy!"); } }); + +linting.lint(); ``` ## Config diff --git a/bin/cli.js b/bin/cli.js index f1aee90..84de543 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -130,6 +130,7 @@ process.on('exit', () => { process.exit(EXIT_CODES.success); } }); + linting.lint(); }) .catch((error) => { logger.error('Failed to lint\n', error); @@ -171,6 +172,7 @@ process.on('exit', () => { onLintingDone(); }); + linting.lint(); }) .catch((error) => { logger.error('Failed to lint file', filePath, '\n', error); diff --git a/src/lib/linting.js b/src/lib/linting.js index b79af47..5ff5a08 100644 --- a/src/lib/linting.js +++ b/src/lib/linting.js @@ -58,8 +58,6 @@ class Linting extends EventEmitter { this.results = {}; /** The logger used to show debugs */ this.logger = logging(`lint:${this.name}`); - - this.lint(); } /** diff --git a/test/api.spec.js b/test/api.spec.js index a7110f8..c566e58 100644 --- a/test/api.spec.js +++ b/test/api.spec.js @@ -10,43 +10,51 @@ const svg = ''; describe('.lintSource()', function () { it('should succeed without config', function (done) { - SVGLint.lintSource(svg).then((result) => { - result.on('done', () => { - expect(result.state).toBe(result.STATES.success); + SVGLint.lintSource(svg).then((linting) => { + linting.on('done', () => { + expect(linting.state).toBe(linting.STATES.success); done(); }); + linting.lint(); }); }); it('should succeed with empty config', function (done) { - SVGLint.lintSource(svg, {}).then((result) => { - result.on('done', () => { - expect(result.state).toBe(result.STATES.success); + SVGLint.lintSource(svg, {}).then((linting) => { + linting.on('done', () => { + expect(linting.state).toBe(linting.STATES.success); done(); }); + linting.lint(); }); }); it('should succeed with empty SVG', function (done) { - SVGLint.lintSource(svg, {}).then((result) => { - result.on('done', () => { - expect(result.state).toBe(result.STATES.success); + SVGLint.lintSource(svg, {}).then((linting) => { + linting.on('done', () => { + expect(linting.state).toBe(linting.STATES.success); done(); }); + linting.lint(); }); }); it('should succeed with empty first line', function (done) { - SVGLint.lintSource('\n' + svg, {}).then((result) => { - result.on('done', () => { - expect(result.state).toBe(result.STATES.success); + SVGLint.lintSource('\n' + svg, {}).then((linting) => { + linting.on('done', () => { + expect(linting.state).toBe(linting.STATES.success); done(); }); + linting.lint(); }); }); it('should throw with malformed SVG', function (done) { - SVGLint.lintSource(' done()); + SVGLint.lintSource(' { + linting.lint(); + }) + .catch(() => done()); }); }); @@ -59,6 +67,7 @@ describe('.lintFile()', function () { linting.on('done', () => { expect(linting.state).toBe(linting.STATES.success); }); + linting.lint(); }); }); @@ -67,6 +76,7 @@ describe('.lintFile()', function () { linting.on('done', () => { expect(linting.state).toBe(linting.STATES.success); }); + linting.lint(); }); }); @@ -78,6 +88,7 @@ describe('.lintFile()', function () { linting.on('done', () => { expect(linting.state).toBe(linting.STATES.success); }); + linting.lint(); }); }); @@ -87,6 +98,7 @@ describe('.lintFile()', function () { linting.on('done', () => { expect(linting.state).toBe(linting.STATES.success); }); + linting.lint(); }, ); }); diff --git a/test/cli.spec.js b/test/cli.spec.js index 1defca6..564097c 100644 --- a/test/cli.spec.js +++ b/test/cli.spec.js @@ -42,7 +42,7 @@ describe('CLI', function () { expect(failed).toBeFalsy(); }); - it('should fail with an invalid SVG', async function () { + it('should fail with a SVG that does not matches config', async function () { const {failed, exitCode} = await execCli( [INVALID_SVG], 'test/projects/with-config', @@ -60,7 +60,7 @@ describe('CLI', function () { expect(failed).toBeFalsy(); }); - it('should fail with an invalid SVG on stdin', async function () { + it('should fail with a SVG that does not matches config on stdin', async function () { const {failed, exitCode} = await execCli( ['--stdin'], 'test/projects/with-config', diff --git a/test/helpers.js b/test/helpers.js index 98e2276..a1fa0e3 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -30,22 +30,6 @@ export function testSucceedsFactory(svg, ruleNameOrConfig) { : ruleNameOrConfig; const linting = await SVGLint.lintSource(svg, config); - // TODO: there is a race condition here. The this.lint() method - // of the Linting class is called in the constructor, so it's possible - // that the linting is already done before we call the on('done') - // event listener. Removing the next condition will make some `valid` - // rules tests fail. - if (linting.state === linting.STATES.success) { - resolve(); - } else if (linting.state !== linting.STATES.linting) { - reject( - new Error( - `Linting failed (${linting.state}):` + - ` ${inspect(config)}`, - ), - ); - } - linting.on('done', () => { if (linting.state === linting.STATES.success) { resolve(); @@ -58,6 +42,8 @@ export function testSucceedsFactory(svg, ruleNameOrConfig) { ); } }); + + linting.lint(); }); }; } @@ -84,18 +70,6 @@ export function testFailsFactory(svg, ruleNameOrConfig) { : ruleNameOrConfig; const linting = await SVGLint.lintSource(svg, config); - // TODO: Same that the TODO explained at testSucceedsFactory - if (linting.state === linting.STATES.error) { - resolve(); - } else if (linting.state !== linting.STATES.linting) { - reject( - new Error( - `Linting did not fail (${linting.state}):` + - ` ${inspect(config)}`, - ), - ); - } - linting.on('done', () => { if (linting.state === linting.STATES.error) { resolve(); @@ -108,6 +82,8 @@ export function testFailsFactory(svg, ruleNameOrConfig) { ); } }); + + linting.lint(); }); }; } diff --git a/test/reporter.spec.js b/test/reporter.spec.js index 9e1769b..9a3ae46 100644 --- a/test/reporter.spec.js +++ b/test/reporter.spec.js @@ -8,6 +8,7 @@ async function lint(source, rules) { linting.on('done', () => { resolve(linting.results); }); + linting.lint(); }); }