Skip to content

🧪 QA: Add tests for common utilities #387

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.

Sign up for GitHub

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

Open
daggerstuff wants to merge 4 commits into
base: staging
Choose a base branch
from
+13 −0
Open

🧪 QA: Add tests for common utilities #387

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d8d4af
🧪 QA: Add tests for common utilities
google-labs-jules[bot] Apr 4, 2026
8507a69
🧪 QA: Add tests for common utilities
google-labs-jules[bot] Apr 4, 2026
ef4edd1
🧪 QA: Add tests for common utilities
google-labs-jules[bot] Apr 4, 2026
30738df
🧪 QA: Add tests for common utilities
google-labs-jules[bot] Apr 4, 2026
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
13 changes: 13 additions & 0 deletions src/utils/common.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { describe, expect, it } from "vitest"
import { unescapeHTML } from "./common"

describe("common utilities", () => {
it("unescapes HTML entities in VNode children", () => {
const node = {
type: "div",
children: "<b>bold</b> & beautiful",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<b>bold</b> & beautiful")
Comment on lines +6 to +11
Copy link
Copy Markdown

@charliecreates charliecreates bot Apr 4, 2026

Choose a reason for hiding this comment

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

The test doesn’t clarify whether unescapeHTML is expected to mutate the input node or return a new node. That behavior matters to callers and is easy to change unintentionally. Consider asserting the intended behavior explicitly (either result === node for mutation or result !== node for immutability).

Suggestion

Add an explicit assertion for mutation vs immutability based on the intended contract, e.g.:

  • If immutable: expect(result).not.toBe(node)
  • If mutating: expect(result).toBe(node)

Optionally, deep-freeze node to ensure immutability is enforced if that’s the goal. Reply with "@CharlieHelps yes please" if you’d like me to add a commit with the assertion that matches the intended behavior.

Comment on lines +1 to +11
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

This test file uses double quotes for imports/strings, but the repo’s formatter config prefers single quotes (see .prettierrc singleQuote:true) and existing utils tests follow that convention. Please reformat (or run pnpm format) so this file matches the enforced style and doesn’t fail format:check.

Suggested change
import { describe, expect, it } from "vitest"
import { unescapeHTML } from "./common"
describe("common utilities", () => {
it("unescapes HTML entities in VNode children", () => {
const node = {
type: "div",
children: "&lt;b&gt;bold&lt;/b&gt; &amp; beautiful",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<b>bold</b> & beautiful")
import { describe, expect, it } from 'vitest'
import { unescapeHTML } from './common'
describe('common utilities', () => {
it('unescapes HTML entities in VNode children', () => {
const node = {
type: 'div',
children: '&lt;b&gt;bold&lt;/b&gt; &amp; beautiful',
}
const result = unescapeHTML(node)
expect(result.children).toBe('<b>bold</b> & beautiful')

Copilot uses AI. Check for mistakes.
})
Comment on lines +5 to +12
Copy link
Copy Markdown

@charliecreates charliecreates bot Apr 4, 2026

Choose a reason for hiding this comment

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

This test only covers the happy path where children is a simple string containing a few entities. If unescapeHTML is meant to be a general utility, it’s easy for regressions to slip in without covering common shapes (e.g., missing/undefined children, array children, nested nodes, already-unescaped strings, or other entities). Adding a small table-driven set of cases would make this much harder to break accidentally.

Suggestion

Expand coverage with a small set of additional cases (table-driven) that reflect the function’s expected contract, e.g.:

  • children missing / null / non-string
  • children array (if supported) and nested nodes
  • idempotency (calling twice yields same result)
  • a few more entities (e.g., &quot;, &#39;, numeric entities) if supported

If you want, reply with "@CharlieHelps yes please" and I’ll add a commit that introduces a describe.each/it.each matrix covering these cases.

})
Comment on lines +4 to +13
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 4, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Expand test coverage to include edge cases and additional entity types.

The PR description states this adds tests "to cover missing edge cases," but currently only one basic happy-path test exists. The unescapeHTML implementation handles several scenarios not covered here:

Missing test cases:

  1. Additional entity types: &quot;, &#039;, &#x2F;, &#x3D; (implementation handles 7 entity types total, test only covers 3)
  2. Array children: Recursive processing of arrays containing strings and VNodes
  3. Nested VNode children: Recursive unescaping of nested object children
  4. Edge cases: null/undefined node, empty children, no children property
📝 Proposed additional test cases to improve coverage 
 describe("common utilities", () => {
   it("unescapes HTML entities in VNode children", () => {
     const node = {
       type: "div",
       children: "&lt;b&gt;bold&lt;/b&gt; &amp; beautiful",
     }
     const result = unescapeHTML(node)
     expect(result.children).toBe("<b>bold</b> & beautiful")
   })
+
+  it("unescapes all supported HTML entities", () => {
+    const node = {
+      type: "div",
+      children: "&lt;&gt;&quot;&#039;&#x2F;&#x3D;&amp;",
+    }
+    const result = unescapeHTML(node)
+    expect(result.children).toBe("<>\"'/=&")
+  })
+
+  it("unescapes HTML entities in array children", () => {
+    const node = {
+      type: "div",
+      children: [
+        "&lt;text&gt;",
+        { type: "span", children: "&amp; more &quot;text&quot;" },
+      ],
+    }
+    const result = unescapeHTML(node)
+    expect(result.children).toEqual([
+      "<text>",
+      { type: "span", children: "& more \"text\"" },
+    ])
+  })
+
+  it("handles null or undefined nodes", () => {
+    expect(unescapeHTML(null as any)).toBe(null)
+    expect(unescapeHTML(undefined as any)).toBe(undefined)
+  })
+
+  it("handles nodes without children", () => {
+    const node = { type: "div" }
+    const result = unescapeHTML(node)
+    expect(result).toEqual({ type: "div" })
+  })
+
+  it("handles deeply nested VNode children", () => {
+    const node = {
+      type: "div",
+      children: {
+        type: "p",
+        children: {
+          type: "span",
+          children: "&lt;nested&gt; &amp; deep",
+        },
+      },
+    }
+    const result = unescapeHTML(node)
+    expect(result.children).toEqual({
+      type: "p",
+      children: {
+        type: "span",
+        children: "<nested> & deep",
+      },
+    })
+  })

Would you like me to open an issue to track these additional test cases?

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
describe("common utilities", () => {
it("unescapes HTML entities in VNode children", () => {
const node = {
type: "div",
children: "&lt;b&gt;bold&lt;/b&gt; &amp; beautiful",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<b>bold</b> & beautiful")
})
})
describe("common utilities", () => {
it("unescapes HTML entities in VNode children", () => {
const node = {
type: "div",
children: "&lt;b&gt;bold&lt;/b&gt; &amp; beautiful",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<b>bold</b> & beautiful")
})
it("unescapes all supported HTML entities", () => {
const node = {
type: "div",
children: "&lt;&gt;&quot;&#039;&#x2F;&#x3D;&amp;",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<>\"'/=&")
})
it("unescapes HTML entities in array children", () => {
const node = {
type: "div",
children: [
"&lt;text&gt;",
{ type: "span", children: "&amp; more &quot;text&quot;" },
],
}
const result = unescapeHTML(node)
expect(result.children).toEqual([
"<text>",
{ type: "span", children: "& more \"text\"" },
])
})
it("handles null or undefined nodes", () => {
expect(unescapeHTML(null as any)).toBe(null)
expect(unescapeHTML(undefined as any)).toBe(undefined)
})
it("handles nodes without children", () => {
const node = { type: "div" }
const result = unescapeHTML(node)
expect(result).toEqual({ type: "div" })
})
it("handles deeply nested VNode children", () => {
const node = {
type: "div",
children: {
type: "p",
children: {
type: "span",
children: "&lt;nested&gt; &amp; deep",
},
},
}
const result = unescapeHTML(node)
expect(result.children).toEqual({
type: "p",
children: {
type: "span",
children: "<nested> & deep",
},
})
})
})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/utils/common.test.ts` around lines 4 - 13, Add expanded tests for
unescapeHTML in src/utils/common.test.ts to cover the missing edge cases: assert
additional entity types (&quot;, &#039;, &#x2F;, &#x3D;) are decoded by
unescapeHTML; verify array children are processed recursively (arrays with
strings and VNode-like objects passed to unescapeHTML); verify nested VNode
children (objects with children that are VNodes) are unescaped recursively; and
add edge-case tests for null/undefined node, empty children, and nodes with no
children property. Reference the unescapeHTML function in your new test cases
and ensure each scenario has an expectation that matches the implementation's
return shape (e.g., result.children is the unescaped string or array).

Loading