Skip to content

test: improve utils test clarity and performance#11

Merged
JosunLP merged 3 commits intodevelopmentfrom
copilot/update-variable-name-in-tests
Jan 24, 2026
Merged

test: improve utils test clarity and performance#11
JosunLP merged 3 commits intodevelopmentfrom
copilot/update-variable-name-in-tests

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 24, 2026

Applied three targeted improvements to tests/utils.test.ts:

  • Variable naming: iattempt in randomInt test loop for clarity
  • Test efficiency: Reduced randomInt iterations from 100 → 20 (sufficient coverage, 5x faster)
  • Security coverage: Added direct assertion that merge() result doesn't inherit polluted properties
// Before
for (let i = 0; i < 100; i++) {
  const result = utils.randomInt(1, 6);
  // ...
}

// After  
for (let attempt = 0; attempt < 20; attempt++) {
  const result = utils.randomInt(1, 6);
  // ...
}

The merge security test now validates both that the prototype isn't polluted AND that the returned object itself doesn't contain the polluted property:

const result = utils.merge({}, malicious);
expect((result as Record<string, unknown>).polluted).toBeUndefined(); // New assertion
expect(({} as Record<string, unknown>).polluted).toBeUndefined();     // Existing
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"[nitpick] The variable name 'i' is a generic loop counter. Consider using a more descriptive name like 'iteration' or 'testRun' to improve readability.","fixFiles":[{"filePath":"tests/utils.test.ts","diff":"diff --git a/tests/utils.test.ts b/tests/utils.test.ts\n--- a/tests/utils.test.ts\n+++ b/tests/utils.test.ts\n@@ -131,7 +131,7 @@\n   });\n \n   it('randomInt returns value in range', () => {\n-    for (let i = 0; i < 100; i++) {\n+    for (let attempt = 0; attempt < 100; attempt++) {\n       const result = utils.randomInt(1, 6);\n       expect(result).toBeGreaterThanOrEqual(1);\n       expect(result).toBeLessThanOrEqual(6);\n"}]},{"message":"This test verifies prototype pollution protection by checking a global property, but it doesn't test that the specific object returned by merge() doesn't contain the polluted property. Consider adding a direct test on the result object.","fixFiles":[{"filePath":"tests/utils.test.ts","diff":"diff --git a/tests/utils.test.ts b/tests/utils.test.ts\n--- a/tests/utils.test.ts\n+++ b/tests/utils.test.ts\n@@ -193,6 +193,9 @@\n     const malicious = JSON.parse('{\"__proto__\": {\"polluted\": true}}');\n     const result = utils.merge({}, malicious);\n \n+    // The result itself should not contain the polluted property\n+    expect((result as Record<string, unknown>).polluted).toBeUndefined();\n+\n     // Should not pollute Object prototype\n     expect(({} as Record<string, unknown>).polluted).toBeUndefined();\n     // The key should not be an own property of the result\n"}]},{"message":"[nitpick] Running randomInt() 100 times in a test may be excessive and could slow down test execution. Consider reducing to 10-20 iterations or using a property-based testing approach.","fixFiles":[{"filePath":"tests/utils.test.ts","diff":"diff --git a/tests/utils.test.ts b/tests/utils.test.ts\n--- a/tests/utils.test.ts\n+++ b/tests/utils.test.ts\n@@ -131,7 +131,7 @@\n   });\n \n   it('randomInt returns value in range', () => {\n-    for (let i = 0; i < 100; i++) {\n+    for (let i = 0; i < 20; i++) {\n       const result = utils.randomInt(1, 6);\n       expect(result).toBeGreaterThanOrEqual(1);\n       expect(result).toBeLessThanOrEqual(6);\n"}]}]

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

JosunLP and others added 2 commits January 24, 2026 01:02
Security and Playground Fixes:
* Replaced direct assignment of HTML via `innerHTML` with safer DOM manipulation in the `logEvent` function within `playground/src/demos/core.ts` to prevent possible HTML injection vulnerabilities.
* Updated collection handling in the same file to use the raw DOM element when re-appending items, improving code clarity and safety.

Workflow and Versioning:
* Added explicit `contents: read` permissions to both `build` and `build-docs` jobs in `.github/workflows/npm-publish.yml` to comply with GitHub Actions best practices. [[1]](diffhunk://#diff-8a5ce8b612395836520d0655143f732d08e747af57f3cfe76b5e283600106240R16-R17) [[2]](diffhunk://#diff-8a5ce8b612395836520d0655143f732d08e747af57f3cfe76b5e283600106240R29-R30)
* Bumped package version to `1.1.1` in `package.json` to reflect the new release.
* Added a changelog entry for version `1.1.1` noting the HTML handling fix in `CHANGELOG.md`.
- Use descriptive variable name 'attempt' instead of generic 'i'
- Reduce randomInt test iterations from 100 to 20 for faster execution
- Add direct test for merge() result not containing polluted property

Co-authored-by: JosunLP <20913954+JosunLP@users.noreply.github.com>
Copilot AI changed the title [WIP] Update variable name for better readability in tests test: improve utils test clarity and performance Jan 24, 2026
Copilot AI requested a review from JosunLP January 24, 2026 00:09
@JosunLP JosunLP changed the base branch from main to development January 24, 2026 00:16
@JosunLP JosunLP marked this pull request as ready for review January 24, 2026 00:16
Copilot AI review requested due to automatic review settings January 24, 2026 00:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates tests/utils.test.ts to improve readability, reduce unnecessary test iterations, and strengthen prototype-pollution assertions around utils.merge().

Changes:

  • Renamed the randomInt loop counter from i to attempt for clarity.
  • Reduced randomInt test iterations from 100 to 20 to speed up the test while keeping the same range validation.
  • Added an assertion that the object returned from merge() does not expose a polluted property.

@JosunLP JosunLP merged commit 74f5f75 into development Jan 24, 2026
12 checks passed
@JosunLP JosunLP deleted the copilot/update-variable-name-in-tests branch January 24, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants