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

fix: Fix race condition when using the API #107

Merged
merged 3 commits into from
Jun 7, 2024
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
1 change: 1 addition & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ jobs:
- 16
- 18
- 20
- 22
include:
- os: macos-latest
node-version: 20
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ linting.on("done", () => {
console.log("You've been a naughty boy!");
}
});

linting.lint();
```

## Config
Expand Down
2 changes: 2 additions & 0 deletions bin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ process.on('exit', () => {
process.exit(EXIT_CODES.success);
}
});
linting.lint();
})
.catch((error) => {
logger.error('Failed to lint\n', error);
Expand Down Expand Up @@ -171,6 +172,7 @@ process.on('exit', () => {

onLintingDone();
});
linting.lint();
})
.catch((error) => {
logger.error('Failed to lint file', filePath, '\n', error);
Expand Down
2 changes: 0 additions & 2 deletions src/lib/linting.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ class Linting extends EventEmitter {
this.results = {};
/** The logger used to show debugs */
this.logger = logging(`lint:${this.name}`);

this.lint();
}

/**
Expand Down
38 changes: 25 additions & 13 deletions test/api.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,43 +10,51 @@ const svg = '<svg></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('<svg<path', {}).catch(() => done());
SVGLint.lintSource('<svg<path', {})
.then((linting) => {
linting.lint();
})
.catch(() => done());
});
});

Expand All @@ -59,6 +67,7 @@ describe('.lintFile()', function () {
linting.on('done', () => {
expect(linting.state).toBe(linting.STATES.success);
});
linting.lint();
});
});

Expand All @@ -67,6 +76,7 @@ describe('.lintFile()', function () {
linting.on('done', () => {
expect(linting.state).toBe(linting.STATES.success);
});
linting.lint();
});
});

Expand All @@ -78,6 +88,7 @@ describe('.lintFile()', function () {
linting.on('done', () => {
expect(linting.state).toBe(linting.STATES.success);
});
linting.lint();
});
});

Expand All @@ -87,6 +98,7 @@ describe('.lintFile()', function () {
linting.on('done', () => {
expect(linting.state).toBe(linting.STATES.success);
});
linting.lint();
},
);
});
Expand Down
4 changes: 2 additions & 2 deletions test/cli.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
32 changes: 4 additions & 28 deletions test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -58,6 +42,8 @@ export function testSucceedsFactory(svg, ruleNameOrConfig) {
);
}
});

linting.lint();
});
};
}
Expand All @@ -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();
Expand All @@ -108,6 +82,8 @@ export function testFailsFactory(svg, ruleNameOrConfig) {
);
}
});

linting.lint();
});
};
}
1 change: 1 addition & 0 deletions test/reporter.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ async function lint(source, rules) {
linting.on('done', () => {
resolve(linting.results);
});
linting.lint();
});
}

Expand Down