Skip to content

Conversation

@Laertes87
Copy link
Collaborator

@Laertes87 Laertes87 commented Dec 8, 2025

Summary by cubic

Add unit tests for dasel, libreoffice, pandoc, and vtracer converters to verify CLI args, options mapping, output writing, logging, and error handling. Coverage includes soffice filters/outdir, pandoc PDF engine, vtracer options, and dasel file writes.

Written for commit 0521af0. Summary will update automatically on new commits.

@github-actions github-actions bot added Test and removed Test labels Dec 8, 2025
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 issues found across 4 files

Prompt for AI agents (all 6 issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="tests/converters/dasel.test.ts">

<violation number="1" location="tests/converters/dasel.test.ts:58">
P1: Missing `await` before `expect(...).rejects`. Without `await`, this assertion won&#39;t be properly evaluated and the test could pass incorrectly.</violation>

<violation number="2" location="tests/converters/dasel.test.ts:63">
P1: Missing `await` before `expect(...).rejects`. Without `await`, this assertion won&#39;t be properly evaluated and the test could pass incorrectly.</violation>
</file>

<file name="tests/converters/vtracer.test.ts">

<violation number="1" location="tests/converters/vtracer.test.ts:54">
P1: Missing `await` before `expect().rejects.toMatch()`. Without `await`, the async test completes before the rejection assertion is evaluated, causing this test to pass even if the promise doesn&#39;t reject or rejects with a different message. Add `await` to properly wait for the assertion.</violation>
</file>

<file name="tests/converters/pandoc.test.ts">

<violation number="1" location="tests/converters/pandoc.test.ts:62">
P1: Missing `await` before `expect(...).rejects`. Without `await`, this test will pass even if the promise doesn&#39;t reject as expected, since the assertion runs asynchronously after the test completes.</violation>
</file>

<file name="tests/converters/libreoffice.test.ts">

<violation number="1" location="tests/converters/libreoffice.test.ts:111">
P1: Missing `await` before `.resolves` assertion. Without awaiting, this test will always pass regardless of what the promise resolves to.</violation>

<violation number="2" location="tests/converters/libreoffice.test.ts:118">
P1: Missing `await` before `.rejects` assertion. Without awaiting, this test will always pass regardless of whether the promise rejects.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

@github-actions github-actions bot added Test and removed Test labels Dec 8, 2025
@github-actions github-actions bot added Test and removed Test labels Dec 9, 2025
@github-actions github-actions bot added Test and removed Test labels Dec 14, 2025
Copy link
Owner

@C4illin C4illin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@C4illin
Copy link
Owner

C4illin commented Dec 14, 2025

I have been thinking about doing tests testing files as well, do you think it would be worth it?

@Laertes87
Copy link
Collaborator Author

I have been thinking about doing tests testing files as well, do you think it would be worth it?

I think it would be worth adding tests for every file that has some crucial logic in it. I mainly started with the converters because they are essentially the core of the app. But the main.ts is still untested. And everything else besides the types could be tested as well.

@C4illin
Copy link
Owner

C4illin commented Dec 14, 2025

I wrote my message a bit confusing, I meant testing the conversion flow calling the converters directly. I am opposed to this since it mostly tests other peoples code and not this project, but also almost all errors that come up are related to some conversion that needs some extra parameter.

I do agree with that testing more of the app would be nice as well :)

@C4illin
Copy link
Owner

C4illin commented Dec 14, 2025

Feel free to merge this when you feel done as well

@Laertes87
Copy link
Collaborator Author

I wrote my message a bit confusing, I meant testing the conversion flow calling the converters directly. I am opposed to this since it mostly tests other peoples code and not this project, but also almost all errors that come up are related to some conversion that needs some extra parameter.

I do agree with that testing more of the app would be nice as well :)

I see, but I think that can be part of the test of the main.ts and other parts? Although that test of the whole application/conversion flow would go towards integration tests rather than unit tests. But then again, integration tests would also be very useful to find errors with new converters.

@Laertes87 Laertes87 merged commit 8f898be into C4illin:main Dec 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants