test: add unit tests for converters#373
Merged
C4illin merged 31 commits intoC4illin:mainfrom Aug 11, 2025
Merged
Conversation
Contributor
Reviewer's GuideThis PR refactors all converter modules to support injection of a mockable execFile function, introduces shared types and test helpers, and adds comprehensive unit test suites for multiple converters to improve testability and coverage. Class diagram for ExecFileFn injection in converter modulesclassDiagram
class ExecFileFn {
<<type>>
+ (cmd: string, args: string[], callback: (err: Error | null, stdout: string, stderr: string) => void, options?: ExecFileOptions) => void
}
class ConvertFnWithExecFile {
<<type>>
+ (filePath: string, fileType: string, convertTo: string, targetPath: string, options: unknown, execFileOverride?: ExecFileFn) => Promise<string>
}
class CalibreConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class AssimpConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class DvisvgmConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class FfmpegConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class GraphicsmagickConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class ImagemagickConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class InkscapeConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class LibheifConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class LibjxlConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class LibreofficeConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class PandocConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class PotraceConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class ResvgConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class VipsConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
class XelatexConverter {
+convert(filePath, fileType, convertTo, targetPath, options?, execFile: ExecFileFn)
}
ExecFileFn <|.. ConvertFnWithExecFile
ConvertFnWithExecFile <|.. CalibreConverter
ConvertFnWithExecFile <|.. AssimpConverter
ConvertFnWithExecFile <|.. DvisvgmConverter
ConvertFnWithExecFile <|.. FfmpegConverter
ConvertFnWithExecFile <|.. GraphicsmagickConverter
ConvertFnWithExecFile <|.. ImagemagickConverter
ConvertFnWithExecFile <|.. InkscapeConverter
ConvertFnWithExecFile <|.. LibheifConverter
ConvertFnWithExecFile <|.. LibjxlConverter
ConvertFnWithExecFile <|.. LibreofficeConverter
ConvertFnWithExecFile <|.. PandocConverter
ConvertFnWithExecFile <|.. PotraceConverter
ConvertFnWithExecFile <|.. ResvgConverter
ConvertFnWithExecFile <|.. VipsConverter
ConvertFnWithExecFile <|.. XelatexConverter
Class diagram for shared test helpers and test structureclassDiagram
class CommonTests {
+runCommonConverterTests(converter, options)
}
class ConverterTestHelpers {
+mockExecFileSuccess
+mockExecFileFailure
+mockExecFileWithStdout
+mockExecFileWithStderr
}
class ConverterTestSuite {
+assimp.test.ts
+calibre.test.ts
+dvisvgm.test.ts
+ffmpeg.test.ts
+graphicsmagick.test.ts
+imagemagick.test.ts
+inkscape.test.ts
+libheif.test.ts
+libjxl.test.ts
+msgconvert.test.ts
+potrace.test.ts
+resvg.test.ts
+vips.test.ts
+xelatex.test.ts
}
ConverterTestHelpers <|-- CommonTests
CommonTests <|-- ConverterTestSuite
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @Laertes87 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/converters/types.ts:1` </location>
<code_context>
+export type ExecFileFn = (
+ cmd: string,
+ args: string[],
+ callback: (err: Error | null, stdout: string, stderr: string) => void,
+) => void;
+
</code_context>
<issue_to_address>
ExecFileFn type omits optional options parameter from execFile signature.
Including the optional options parameter in ExecFileFn will ensure compatibility with all execFile use cases, such as setting environment variables or working directories.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
export type ExecFileFn = (
cmd: string,
args: string[],
callback: (err: Error | null, stdout: string, stderr: string) => void,
) => void;
=======
export type ExecFileFn = (
cmd: string,
args: string[],
options: import('child_process').ExecFileOptions | undefined,
callback: (err: Error | null, stdout: string, stderr: string) => void,
) => void;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/converters/assimp.ts:121` </location>
<code_context>
targetPath: string,
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
options?: unknown,
+ execFile: ExecFileFn = execFileOriginal, // to make it mockable
): Promise<string> {
return new Promise((resolve, reject) => {
</code_context>
<issue_to_address>
execFile parameter is added as a positional argument, which may cause issues with existing function calls.
Consider using an options object or placing execFile as the last parameter to avoid breaking existing calls that use positional arguments.
</issue_to_address>
### Comment 3
<location> `src/converters/dvisvgm.ts:19` </location>
<code_context>
targetPath: string,
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
options?: unknown,
+ execFile: ExecFileFn = execFileOriginal, // to make it mockable
): Promise<string> {
return new Promise((resolve, reject) => {
</code_context>
<issue_to_address>
execFile parameter is added as a positional argument, which may break existing usage.
</issue_to_address>
### Comment 4
<location> `src/converters/ffmpeg.ts:696` </location>
<code_context>
targetPath: string,
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
options?: unknown,
+ execFile: ExecFileFn = execFileOriginal, // to make it mockable
): Promise<string> {
return new Promise((resolve, reject) => {
</code_context>
<issue_to_address>
execFile parameter is added as a positional argument, which may break existing usage.
</issue_to_address>
### Comment 5
<location> `tests/converters/imagemagick.test.ts:21` </location>
<code_context>
+ await runConvertSuccessTest(convert);
+});
+
+test("convert rejects when execFile fails", async () => {
+ await runConvertFailTest(convert);
+});
</code_context>
<issue_to_address>
Consider adding a test for when both stdout and stderr are present.
Adding such a test will help verify that both outputs are handled correctly when they occur together.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @Laertes87 - I've reviewed your changes - here's some feedback:
- A lot of the execFile promise and logging boilerplate is duplicated across converters—consider extracting it into a shared helper or higher-order function to reduce repetition.
- The ExecFileFn signature currently takes an
unknownoptions parameter—narrowing that to the actual ExecFileOptions type would improve type safety and clarity. - You’ve added tests for several converters—plan to cover the remaining converters (e.g., calibre, assimp, graphicsmagick) in this PR or a follow-up to keep test coverage consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- A lot of the execFile promise and logging boilerplate is duplicated across converters—consider extracting it into a shared helper or higher-order function to reduce repetition.
- The ExecFileFn signature currently takes an `unknown` options parameter—narrowing that to the actual ExecFileOptions type would improve type safety and clarity.
- You’ve added tests for several converters—plan to cover the remaining converters (e.g., calibre, assimp, graphicsmagick) in this PR or a follow-up to keep test coverage consistent.
## Individual Comments
### Comment 1
<location> `src/converters/types.ts:1` </location>
<code_context>
+export type ExecFileFn = (
+ cmd: string,
+ args: string[],
+ options: import("child_process").ExecFileOptions | unknown | undefined | null,
+ callback: (err: Error | null, stdout: string, stderr: string) => void,
+) => void;
+
+export type ConvertFnWithExecFile = (
</code_context>
<issue_to_address>
The ExecFileFn type's options parameter is too permissive.
Restrict the options parameter to ExecFileOptions or undefined for better type safety and to prevent potential runtime issues.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
export type ExecFileFn = (
cmd: string,
args: string[],
options: import("child_process").ExecFileOptions | unknown | undefined | null,
callback: (err: Error | null, stdout: string, stderr: string) => void,
) => void;
=======
export type ExecFileFn = (
cmd: string,
args: string[],
options?: import("child_process").ExecFileOptions,
callback: (err: Error | null, stdout: string, stderr: string) => void,
) => void;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/converters/ffmpeg.test.ts:143` </location>
<code_context>
+ expect(loggedMessage).toBe("stdout: Fake stdout");
+});
+
+test("fails on exec error", async () => {
+ const originalConsoleError = console.error;
+
+ let loggedMessage = "";
+ console.error = (msg) => {
+ loggedMessage = msg;
+ };
+
+ expect(convert("fail.mov", "mov", "mp4", "output.mp4", undefined, mockExecFile)).rejects.toThrow(
+ "mock failure",
+ );
+
+ console.error = originalConsoleError;
+
+ expect(loggedMessage).toBe("stderr: Fake stderr: fail");
+});
</code_context>
<issue_to_address>
Missing test for stderr-only output when execFile does not error.
Please add a test where execFile returns only stderr output without an error to verify correct stderr logging on successful conversions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
test("fails on exec error", async () => {
const originalConsoleError = console.error;
let loggedMessage = "";
console.error = (msg) => {
loggedMessage = msg;
};
expect(convert("fail.mov", "mov", "mp4", "output.mp4", undefined, mockExecFile)).rejects.toThrow(
"mock failure",
);
console.error = originalConsoleError;
expect(loggedMessage).toBe("stderr: Fake stderr: fail");
});
=======
test("fails on exec error", async () => {
const originalConsoleError = console.error;
let loggedMessage = "";
console.error = (msg) => {
loggedMessage = msg;
};
expect(convert("fail.mov", "mov", "mp4", "output.mp4", undefined, mockExecFile)).rejects.toThrow(
"mock failure",
);
console.error = originalConsoleError;
expect(loggedMessage).toBe("stderr: Fake stderr: fail");
});
test("logs stderr when execFile returns only stderr and no error", async () => {
const originalConsoleError = console.error;
let loggedMessage = "";
console.error = (msg) => {
loggedMessage = msg;
};
// Mock execFile to call back with no error, no stdout, but with stderr
const mockExecFileStderrOnly = (
_cmd: string,
_args: string[],
_opts: any,
cb: (err: Error | null, stdout: string, stderr: string) => void,
) => {
cb(null, "", "Only stderr output");
};
await convert("input.mov", "mov", "mp4", "output.mp4", undefined, mockExecFileStderrOnly);
console.error = originalConsoleError;
expect(loggedMessage).toBe("stderr: Only stderr output");
});
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/converters/helpers/converters.ts:30` </location>
<code_context>
+ expect(loggedMessage).toBe("stdout: Fake stdout");
+}
+
+export async function runConvertFailTest(convertFn: ConvertFnWithExecFile) {
+ const mockExecFile: ExecFileFn = (
+ _cmd: string,
+ _args: string[],
+ options: unknown,
+ callback: (err: ExecFileException | null, stdout: string, stderr: string) => void,
+ ) => {
+ callback(new Error("Test error"), "", "");
+ };
+
+ expect(
+ convertFn("input.obj", "obj", "stl", "output.stl", undefined, mockExecFile),
+ ).rejects.toMatch(/error: Error: Test error/);
</code_context>
<issue_to_address>
No test for error object with missing message property.
Add a test case where the error object lacks a 'message' property or isn't an Error instance to verify robust error handling for non-standard error shapes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
expect(
convertFn("input.obj", "obj", "stl", "output.stl", undefined, mockExecFile),
).rejects.toMatch(/error: Error: Test error/);
}
=======
expect(
convertFn("input.obj", "obj", "stl", "output.stl", undefined, mockExecFile),
).rejects.toMatch(/error: Error: Test error/);
// Test with error object lacking 'message' property
const mockExecFileNoMessage: ExecFileFn = (
_cmd: string,
_args: string[],
options: unknown,
callback: (err: ExecFileException | null, stdout: string, stderr: string) => void,
) => {
// Simulate a non-standard error object
callback({ notMessage: true } as unknown as ExecFileException, "", "");
};
await expect(
convertFn("input.obj", "obj", "stl", "output.stl", undefined, mockExecFileNoMessage),
).rejects.toMatch(/error:/i);
// Test with a non-object error (e.g., a string)
const mockExecFileStringError: ExecFileFn = (
_cmd: string,
_args: string[],
options: unknown,
callback: (err: ExecFileException | null, stdout: string, stderr: string) => void,
) => {
callback("string error" as unknown as ExecFileException, "", "");
};
await expect(
convertFn("input.obj", "obj", "stl", "output.stl", undefined, mockExecFileStringError),
).rejects.toMatch(/error:/i);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `tests/converters/libjxl.test.ts:34` </location>
<code_context>
+ await runConvertLogsStderrorAndStdout(convert);
+});
+
+test("convert uses djxl with input filetype being jxl", async () => {
+ const originalConsoleLog = console.log;
+
+ let loggedMessage = "";
+ console.log = (msg) => {
+ loggedMessage = msg;
+ };
+
+ const mockExecFile: ExecFileFn = (
+ _cmd: string,
+ _args: string[],
+ options: unknown,
+ callback: (err: ExecFileException | null, stdout: string, stderr: string) => void,
+ ) => {
+ command = _cmd;
+ callback(null, "Fake stdout", "");
+ };
+
+ const result = await convert("input.jxl", "jxl", "png", "output.png", undefined, mockExecFile);
+
+ console.log = originalConsoleLog;
+
+ expect(result).toBe("Done");
+ expect(command).toEqual("djxl");
+ expect(loggedMessage).toBe("stdout: Fake stdout");
+});
+
</code_context>
<issue_to_address>
No test for both fileType and convertTo being non-jxl.
Please add a test case where both fileType and convertTo are not 'jxl' to ensure the converter behaves correctly in this scenario.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @Laertes87 - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
|
Great idea! Will have to look more into this |
Laertes87
commented
Jul 27, 2025
| execFile: ExecFileFn = execFileOriginal, // to make it mockable | ||
| ): Promise<string> { | ||
| let tool = ""; | ||
| if (fileType === "jxl") { |
Collaborator
Author
There was a problem hiding this comment.
@C4illin What I noticed here when writing the tests: If both fileType and convertTo are not jxl, the value of tool is an empty string. Now I didn't dig too deep into the code to see if that would actually be possible. But if it's possible in any circumstance, it might be worth to consider some sort of error handling at this point. Unless of course passing an empty string is perfectly fine and actually intended. Then disregard that.
Owner
There was a problem hiding this comment.
One of them must always be jxl for the tools to work, and in the ui it should only be possible to select jxl if you upload a non-jxl image and vice versa. But some error would be more clear
Owner
|
Would it be possible to replace |
Collaborator
Author
Yeah, probably. I'll take care of it when I get back from vacation next week. |
Laertes87
force-pushed
the
AddUnitTests
branch
from
3561516 to
c010588
Compare
Collaborator
Author
|
I changed the conventional commit keyword to |
Laertes87
changed the title
Owner
Thanks! |
Owner
|
Thanks for this pr! |
evil9369
pushed a commit
to pi-docket/ConvertX-CN
that referenced
this pull request
Feb 1, 2026
test: add unit tests for converters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I thought I'd start with some unit tests for the converters. At least for the less complex ones so far. If I have more time in the next few days/weeks, I could also tackle the more complex ones that are still missing.
Run tests with
bun testorbun test --coverageto see the code coverage of the tests.Might also want to consider creating a workflow so that the tests have to pass for every pull requests, i.e. block merging if the tests fail.
Summary by Sourcery
Enable mockable execFile calls in all converter modules and bolster coverage with shared helpers and comprehensive unit tests for multiple converters
New Features:
Enhancements:
Tests: