Conversation
There was a problem hiding this comment.
3 issues found across 42 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/pages/firstRun.test.ts">
<violation number="1" location="tests/pages/firstRun.test.ts:30">
P3: These assertions assume default env: `WEBROOT` unset and `ALLOW_UNAUTHENTICATED` false, but `tests/pages/helpers/preload.ts` pins only the DB/file paths. Running the suite from a shell where `WEBROOT` or `ALLOW_UNAUTHENTICATED` (both documented vars) is set makes the location assertions (`${WEBROOT}/setup`, and the home page skips the FIRST_RUN redirect when ALLOW_UNAUTHENTICATED is true) and `action="/register"` fail. Pin `WEBROOT=""` and `ALLOW_UNAUTHENTICATED="false"` in the preload so the page tests are hermetic.</violation>
</file>
<file name="tests/pages/listConverters.test.ts">
<violation number="1" location="tests/pages/listConverters.test.ts:18">
P3: Test 2 derives its expectations from the same getAllTargets()/getAllInputs() the page iterates, so if converter discovery in src/converters/main.ts drops a converter (or an entry loses its inputs/targets), both the page and the test lose it together and the test still passes. Build the expected counts from the raw `properties` maps instead of from the functions whose behavior the page is being tested against, so a broken discovery regression fails the test.</violation>
</file>
<file name="tests/pages/results.test.ts">
<violation number="1" location="tests/pages/results.test.ts:167">
P2: This assertion fails unless @kitajs/html auto-prepends a doctype: `BaseHtml` (src/components/base.tsx) starts the document with `<html lang="en">` and no `<!doctype html>` is emitted anywhere, so the GET response body begins with `<html lang="en">`. Assert on the actual outer tag instead, e.g. `toStartWith('<html lang="en">')`.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
|
||
| const body = await (await request(`/results/${job}`, { cookies: { auth: user.token } })).text(); | ||
|
|
||
| expect(body).toStartWith("<!doctype html>"); |
There was a problem hiding this comment.
P2: This assertion fails unless @kitajs/html auto-prepends a doctype: BaseHtml (src/components/base.tsx) starts the document with <html lang="en"> and no <!doctype html> is emitted anywhere, so the GET response body begins with <html lang="en">. Assert on the actual outer tag instead, e.g. toStartWith('<html lang="en">').
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/pages/results.test.ts, line 167:
<comment>This assertion fails unless @kitajs/html auto-prepends a doctype: `BaseHtml` (src/components/base.tsx) starts the document with `<html lang="en">` and no `<!doctype html>` is emitted anywhere, so the GET response body begins with `<html lang="en">`. Assert on the actual outer tag instead, e.g. `toStartWith('<html lang="en">')`.</comment>
<file context>
@@ -10,3 +15,171 @@ test("preserves output path segments while encoding the filename", () => {
+
+ const body = await (await request(`/results/${job}`, { cookies: { auth: user.token } })).text();
+
+ expect(body).toStartWith("<!doctype html>");
+ expect(body).toContain("<title>ConvertX | Result</title>");
+ expect(body).toContain('<script src="/results.js" defer');
</file context>
| expect(body).toStartWith("<!doctype html>"); | |
| expect(body).toStartWith('<html lang="en">'); |
There was a problem hiding this comment.
Not valid and not fixed. @elysiajs/html sets autoDoctype ??= true and adds <!doctype html> to any response that starts with <html. The test passes as written.
|
|
||
| test("sends visitors of the home page to the setup", () => { | ||
| expect(firstRun.home.status).toBe(302); | ||
| expect(firstRun.home.location).toBe("/setup"); |
There was a problem hiding this comment.
P3: These assertions assume default env: WEBROOT unset and ALLOW_UNAUTHENTICATED false, but tests/pages/helpers/preload.ts pins only the DB/file paths. Running the suite from a shell where WEBROOT or ALLOW_UNAUTHENTICATED (both documented vars) is set makes the location assertions (${WEBROOT}/setup, and the home page skips the FIRST_RUN redirect when ALLOW_UNAUTHENTICATED is true) and action="/register" fail. Pin WEBROOT="" and ALLOW_UNAUTHENTICATED="false" in the preload so the page tests are hermetic.
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/pages/firstRun.test.ts, line 30:
<comment>These assertions assume default env: `WEBROOT` unset and `ALLOW_UNAUTHENTICATED` false, but `tests/pages/helpers/preload.ts` pins only the DB/file paths. Running the suite from a shell where `WEBROOT` or `ALLOW_UNAUTHENTICATED` (both documented vars) is set makes the location assertions (`${WEBROOT}/setup`, and the home page skips the FIRST_RUN redirect when ALLOW_UNAUTHENTICATED is true) and `action="/register"` fail. Pin `WEBROOT=""` and `ALLOW_UNAUTHENTICATED="false"` in the preload so the page tests are hermetic.</comment>
<file context>
@@ -0,0 +1,55 @@
+
+ test("sends visitors of the home page to the setup", () => {
+ expect(firstRun.home.status).toBe(302);
+ expect(firstRun.home.location).toBe("/setup");
+ });
+
</file context>
There was a problem hiding this comment.
Valid, and broader than reported. tests/preload.ts now unsets all 10 settings that src/helpers/env.ts reads. Bun also loads .env files automatically, so this matters even with a clean shell. Before the fix, WEBROOT=/convertx broke 14 page tests and ALLOW_AUTHENTICATED=true broke 7. With the fix, the full suite passes with each variable set.
|
|
||
| const body = await (await request("/converters", { cookies: { auth: user.token } })).text(); | ||
|
|
||
| for (const [converter, targets] of Object.entries(getAllTargets())) { |
There was a problem hiding this comment.
P3: Test 2 derives its expectations from the same getAllTargets()/getAllInputs() the page iterates, so if converter discovery in src/converters/main.ts drops a converter (or an entry loses its inputs/targets), both the page and the test lose it together and the test still passes. Build the expected counts from the raw properties maps instead of from the functions whose behavior the page is being tested against, so a broken discovery regression fails the test.
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/pages/listConverters.test.ts, line 18:
<comment>Test 2 derives its expectations from the same getAllTargets()/getAllInputs() the page iterates, so if converter discovery in src/converters/main.ts drops a converter (or an entry loses its inputs/targets), both the page and the test lose it together and the test still passes. Build the expected counts from the raw `properties` maps instead of from the functions whose behavior the page is being tested against, so a broken discovery regression fails the test.</comment>
<file context>
@@ -0,0 +1,35 @@
+
+ const body = await (await request("/converters", { cookies: { auth: user.token } })).text();
+
+ for (const [converter, targets] of Object.entries(getAllTargets())) {
+ expect(body).toContain(
+ `<td>${converter}</td><td>Count: ${getAllInputs(converter).length}<ul>`,
</file context>
There was a problem hiding this comment.
Not valid and not fixed. Won't fix. Discovery regressions are already caught in tests/converters/main.test.ts: it lists all 21 converter names explicitly and checks the merged inputs and targets. This test only checks that the page renders what the registry returns, and the vcf test pins one row independently. The raw properties map in main.ts isn't exported. The converters' own properties arrays are also mutated when the module loads (the known bug tracked by a .failing test in main.test.ts), so counts built from them would be wrong.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
@C4illin This is a massive PR. And full disclosure here: I let an Opus 5.5 agent run with the task to check the present unit tests with the goal to improve and extend them for cases that have not been covered yet. Then I also let that agent write tests for pages.
Actual production code was not altered, but the agent found 7 bugs and documented them in the code (you can find them by searching for
test.failingwithin the code.To be completely honest, I haven't checked all the changes the agent made. The code I did check looks fine and looking at the code and function coverage report generated by
bun testI assume that the code is at least not bad and actually improved the overall test coverage.I didn't let the agent fix the bugs it found since I thought it would be worth to verify the bugs first and create separate issues for them. If those are valid bugs, they probably should be fixed at some point so that the respective tests are not expected to fail for all eternity.
If you are completely against this approach, I can totally understand that. A proper review of that amount of changes is nearly impossible after all.
Summary by cubic
Extends the unit test suite with new converter, service, and page tests. Production code is unchanged; the work documents 7 bugs as
test.failingmarkers that should be verified and fixed in separate issues.Test infrastructure
tests/preload.tsredirects every test to a temporary database and upload/output directories so tests never touch production data, and pins the runtime timezone to UTC.tests/pages/helpers spin up the real routes against a mock app, covering login, upload, conversion, download, history, and job-deletion flows.Written for commit 83960be. Summary will update on new commits.