Repository navigation
Conversation
|
Is this a bad idea? |
|
It's not a bad idea. But those integration tests probably should be executed inside the docker container and not in the repository itself on the development host. I'm not sure how to do that exactly, but when I run |
|
I moved the pathIgnorePatterns to the bunfig.toml. The argument for the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 436 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/integration/converters/ffmpeg.test.ts">
<violation number="1" location="tests/integration/converters/ffmpeg.test.ts:6">
P2: This golden test inherits `FFMPEG_ARGS` and `FFMPEG_OUTPUT_ARGS`, so it is environment-dependent and can compare output from a different ffmpeg command than the checked-in JPEG. Clear and restore both variables around the conversion, or make `runGoldenConversion` isolate them for all real-converter tests.</violation>
</file>
<file name="tests/integration/converters/outputs/inkscape/sample.pov">
<violation number="1" location="tests/integration/converters/outputs/inkscape/sample.pov:30">
P2: This fixture contains no exported POV geometry, so `inkscape:pov` passes the all-formats test despite producing no representation of the input image. Use a vector input for this target or strengthen the test and classify raster-to-POV output as unsupported instead of treating comment-only output as success.</violation>
</file>
<file name="tests/integration/converters/outputs/graphicsmagick/sample.uil">
<violation number="1" location="tests/integration/converters/outputs/graphicsmagick/sample.uil:45">
P2: This fixture is malformed UIL: the color table is never closed, and every icon pixel uses undeclared symbol `g`. Close the table and map the final color to `g` before declaring `sample_icon`.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
View guided diff | Turn on auto-fix | Re-trigger cubic
| import { runGoldenConversion } from "./golden"; | ||
|
|
||
| test("converts the sample PNG to the expected JPEG bytes", () => | ||
| runGoldenConversion("ffmpeg", convert, "sample.png", "jpg")); |
There was a problem hiding this comment.
P2: This golden test inherits FFMPEG_ARGS and FFMPEG_OUTPUT_ARGS, so it is environment-dependent and can compare output from a different ffmpeg command than the checked-in JPEG. Clear and restore both variables around the conversion, or make runGoldenConversion isolate them for all real-converter tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/integration/converters/ffmpeg.test.ts, line 6:
<comment>This golden test inherits `FFMPEG_ARGS` and `FFMPEG_OUTPUT_ARGS`, so it is environment-dependent and can compare output from a different ffmpeg command than the checked-in JPEG. Clear and restore both variables around the conversion, or make `runGoldenConversion` isolate them for all real-converter tests.</comment>
<file context>
@@ -0,0 +1,6 @@
+import { runGoldenConversion } from "./golden";
+
+test("converts the sample PNG to the expected JPEG bytes", () =>
+ runGoldenConversion("ffmpeg", convert, "sample.png", "jpg"));
</file context>
| /*################################################################### | ||
| ## Exports in this file | ||
| ##========================== | ||
| ## Shapes : 0 |
There was a problem hiding this comment.
P2: This fixture contains no exported POV geometry, so inkscape:pov passes the all-formats test despite producing no representation of the input image. Use a vector input for this target or strengthen the test and classify raster-to-POV output as unsupported instead of treating comment-only output as success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/integration/converters/outputs/inkscape/sample.pov, line 30:
<comment>This fixture contains no exported POV geometry, so `inkscape:pov` passes the all-formats test despite producing no representation of the input image. Use a vector input for this target or strengthen the test and classify raster-to-POV output as unsupported instead of treating comment-only output as success.</comment>
<file context>
@@ -0,0 +1,43 @@
+/*###################################################################
+## Exports in this file
+##==========================
+## Shapes : 0
+## Segments : 0
+## Nodes : 0
</file context>
| color('#9F9FBF00',foreground) = 'a', | ||
| color('#AAB8D500',foreground) = 's', | ||
| color('#BEC0C400',foreground) = 'd', | ||
| color('#C2C4C800',foreground) = 'f', |
There was a problem hiding this comment.
P2: This fixture is malformed UIL: the color table is never closed, and every icon pixel uses undeclared symbol g. Close the table and map the final color to g before declaring sample_icon.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/integration/converters/outputs/graphicsmagick/sample.uil, line 45:
<comment>This fixture is malformed UIL: the color table is never closed, and every icon pixel uses undeclared symbol `g`. Close the table and map the final color to `g` before declaring `sample_icon`.</comment>
<file context>
@@ -0,0 +1,62 @@
+ color('#9F9FBF00',foreground) = 'a',
+ color('#AAB8D500',foreground) = 's',
+ color('#BEC0C400',foreground) = 'd',
+ color('#C2C4C800',foreground) = 'f',
+ sample_icon : icon(color_table = sample_ct,
+ "gggggggggggggggg",
</file context>
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/converters/ffmpeg.test.ts">
<violation number="1" location="tests/converters/ffmpeg.test.ts:120">
P3: This adds the same parameterized test twice, so the suite does redundant work without increasing coverage. Remove the duplicate case (or change it to the distinct scenario this commit intended to cover).</violation>
</file>
<file name="tests/integration/converters/all-formats.test.ts">
<violation number="1" location="tests/integration/converters/all-formats.test.ts:98">
P2: These counts are inconsistent with the expected-failure fixture, so the test cannot pass when all conversions behave as expected. With `testedTargets` set to 405 and 33 expected failures, `successfulTargets` must be 372, not 370.</violation>
</file>
<file name="src/converters/ffmpeg.ts">
<violation number="1" location="src/converters/ffmpeg.ts:205">
P2: `lrc` is classified as both audio and subtitle input, so converter discovery advertises audio, image, and video outputs for lyrics files. Remove `lrc` from `audioInputs` and keep it only in `subtitleInputs`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } | ||
|
|
||
| expect(testedTargets).toBe(405); | ||
| expect(successfulTargets).toBe(370); |
There was a problem hiding this comment.
P2: These counts are inconsistent with the expected-failure fixture, so the test cannot pass when all conversions behave as expected. With testedTargets set to 405 and 33 expected failures, successfulTargets must be 372, not 370.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/integration/converters/all-formats.test.ts, line 98:
<comment>These counts are inconsistent with the expected-failure fixture, so the test cannot pass when all conversions behave as expected. With `testedTargets` set to 405 and 33 expected failures, `successfulTargets` must be 372, not 370.</comment>
<file context>
@@ -94,8 +94,8 @@ test("converts every saved PNG target for each converter", async () => {
- expect(testedTargets).toBe(558);
- expect(successfulTargets).toBe(415);
+ expect(testedTargets).toBe(405);
+ expect(successfulTargets).toBe(370);
expect(observedFailures.sort()).toEqual(expectedFailures);
expect(failures).toEqual([]);
</file context>
| expect(successfulTargets).toBe(370); | |
| expect(successfulTargets).toBe(372); |
| "latm", | ||
| "lbc", | ||
| "loas", | ||
| "lrc", |
There was a problem hiding this comment.
P2: lrc is classified as both audio and subtitle input, so converter discovery advertises audio, image, and video outputs for lyrics files. Remove lrc from audioInputs and keep it only in subtitleInputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/converters/ffmpeg.ts, line 205:
<comment>`lrc` is classified as both audio and subtitle input, so converter discovery advertises audio, image, and video outputs for lyrics files. Remove `lrc` from `audioInputs` and keep it only in `subtitleInputs`.</comment>
<file context>
@@ -18,695 +18,811 @@ type FfmpegExecFile = (
+ "latm",
+ "lbc",
+ "loas",
+ "lrc",
+ "m2a",
+ "m4a",
</file context>
| expect(calls[0]).toEqual(["-i", "in.mp3", "-vn", `out.${ext}`]); | ||
| }); | ||
|
|
||
| test.each(["png", "jpg", "jpeg", "webp"])( |
There was a problem hiding this comment.
P3: This adds the same parameterized test twice, so the suite does redundant work without increasing coverage. Remove the duplicate case (or change it to the distinct scenario this commit intended to cover).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/converters/ffmpeg.test.ts, line 120:
<comment>This adds the same parameterized test twice, so the suite does redundant work without increasing coverage. Remove the duplicate case (or change it to the distinct scenario this commit intended to cover).</comment>
<file context>
@@ -117,6 +117,24 @@ test.each(["wav", "aac"])("drops video streams for %s output", async (ext) => {
expect(calls[0]).toEqual(["-i", "in.mp3", "-vn", `out.${ext}`]);
});
+test.each(["png", "jpg", "jpeg", "webp"])(
+ "drops audio streams when extracting cover art from audio to %s",
+ async (ext) => {
</file context>
Summary by cubic
Adds integration tests for the converter pipeline, covering every format target per converter and golden byte comparisons for sample conversions. The all-formats test asserts each declared target either produces a non-empty file or is listed in
expected-failures.json, and the golden tests compare output byte-for-byte against checked-in reference files. No production code changes.outputsdirectory in ESLint, Prettier, and knip configs so generated binary files stay out of linting.testscript intobunfig.toml.Written for commit e40a79f. Summary will update on new commits.