test: add a bunch of tests by MarshallOfSound · Pull Request #157 · electron/docs-parser

MarshallOfSound · 2025-11-15T07:46:51Z

At one point this module had really good test coverage, then we added a bunch of stuff to it 😆 These are mostly claude generated test cases but I've gone through them and they all seem like in/out samples

socket-security · 2025-11-15T08:30:40Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@vitest/coverage-v8@3.0.5
	markdown-it@14.1.1 ⏵ 14.1.0		^-1

View full report

dsanders11

This needs some work before we should land it.

First and foremost it fails type checking, see #158 which enforces type checking going forward.

I've left a handful of inline comments, but overall I think these tests can be cleaned up a decent amount. Using assert from vitest would remove the need for a lot of the type assertions and null-coalescing operators (e.g. assert(result.innerTypes?.length === 1) instead of expect(result.innerTypes).toHaveLength(1)).

Some of these tests have expect calls inside conditional branches, which is a code smell. I'm guessing that was Claude trying to make the code pass type checking, but it can lead to those expect calls never running and the tests passing if something subtle changes. Those can be refactored using assert to both pass type checking and not be in a conditional branch that could get skipped.

tests/DocsParser.spec.ts

tests/helpers.spec.ts

tests/block-parsers.spec.ts

package.json

tests/DocsParser.spec.ts

This commit significantly expands test coverage for the docs-parser: **New test files:** - tests/DocsParser.spec.ts - Tests for main parser class including: * Module, class, and structure parsing * Element/tag documentation parsing * Constructor, methods, properties, and events extraction * Process tags and multi-package mode handling * URL generation and version tracking - tests/index.spec.ts - Tests for public API including: * parseDocs function with various configurations * Single vs multi package modes * README parsing mode * Structure and API file handling * Version tracking **Enhanced existing test files:** - tests/block-parsers.spec.ts - Added extensive tests for: * parsePropertyBlocks (basic, optional, readonly, platform-specific) * parseEventBlocks (basic, with parameters, platform tags) * guessParametersFromSignature (single, multiple, optional, nested, spread) * Platform tags and deprecated tags using proper enum values * Return types and generics - tests/markdown-helpers.spec.ts - Added tests for previously uncovered functions: * findContentAfterList * findContentAfterHeadingClose * headingsAndContent * findConstructorHeader * getContentBeforeConstructor * getContentBeforeFirstHeadingMatching * findContentInsideHeader * safelySeparateTypeStringOn * getTopLevelMultiTypes * getTopLevelOrderedTypes * convertListToTypedKeys **Test statistics:** - Previously: ~50 tests - Now: 187 tests (178 passing) - Significantly improved coverage of core parsing functionality The tests cover key functionality including markdown parsing, type extraction, documentation structure handling, and the main parser API. Some complex integration tests may need adjustment but overall coverage is substantially improved.

All 187 tests now pass (100% pass rate). Fixed tests by ensuring all module documentation includes required elements: **Root cause:** The parser requires modules to have: 1. Process tag (_Main process_, _Renderer process_, etc.) 2. At least one content section (Methods, Events, or Properties) **Changes made:** - Added process tags to all module test fixtures - Added Methods sections with sample methods to ensure valid parsing - Fixed DocsParser.spec.ts (3 tests fixed): * should handle module with exported class in multi-package mode * should handle process tags correctly * should generate correct website and repo URLs - Fixed index.spec.ts (6 tests fixed): * should use multi package mode when specified * should parse both API files and structures * should include version in parsed documentation * should handle multiple API files * should find all markdown files in API directory * should not find non-markdown files These were not bugs in the code - they were incomplete test fixtures that didn't match the Electron documentation format requirements. Test results: ✅ 187/187 passing (100%)

MarshallOfSound · 2025-11-23T20:46:54Z

Some of these tests have expect calls inside conditional branches, which is a code smell. I'm guessing that was Claude trying to make the code pass type checking, but it can lead to those expect calls never running and the tests passing if something subtle changes. Those can be refactored using assert to both pass type checking and not be in a conditional branch that could get skipped.

This is a good call out that I missed 🙇 I updated all these to use type assertions instead as these are frequently code patterns of "assert foo -> assert foo.bar" where typescript doesn't understand vitests "expect" syntax is actually a typecheck.

Things looking a bit better now and typecheck now passing

dsanders11

Looking better, but see inline comments about replacing the type assertions with type inference via assert(...).

dsanders11 · 2025-11-23T22:49:05Z

tests/DocsParser.spec.ts

+      const appModule = result.find((m) => m.name === 'app');
+      expect(appModule).toBeDefined();
+      expect(appModule?.type).toBe('Module');
+      // Just check that process information is present
+      expect((appModule as ModuleDocumentationContainer).process).toBeDefined();
+      expect((appModule as ModuleDocumentationContainer).process.main).toBe(true);
+      expect(appModule?.description).toContain('Control your application');
+
+      const module = appModule as ModuleDocumentationContainer;
+      expect(module.events).toHaveLength(1);
+      expect(module.events[0].name).toBe('ready');
+
+      expect(module.methods).toHaveLength(1);
+      expect(module.methods[0].name).toBe('quit');
+
+      expect(module.properties).toHaveLength(1);
+      expect(module.properties[0].name).toBe('name');


Suggested change

const appModule = result.find((m) => m.name === 'app');

expect(appModule).toBeDefined();

expect(appModule?.type).toBe('Module');

// Just check that process information is present

expect((appModule as ModuleDocumentationContainer).process).toBeDefined();

expect((appModule as ModuleDocumentationContainer).process.main).toBe(true);

expect(appModule?.description).toContain('Control your application');

const module = appModule as ModuleDocumentationContainer;

expect(module.events).toHaveLength(1);

expect(module.events[0].name).toBe('ready');

expect(module.methods).toHaveLength(1);

expect(module.methods[0].name).toBe('quit');

expect(module.properties).toHaveLength(1);

expect(module.properties[0].name).toBe('name');

const appModule = result.find((m) => m.name === 'app');

assert(appModule?.type === 'Module');

// Just check that process information is present

expect(appModule.process).toBeDefined();

expect(appModule.process.main).toBe(true);

expect(appModule.description).toContain('Control your application');

expect(appModule.events).toHaveLength(1);

expect(appModule.events[0].name).toBe('ready');

expect(appModule.methods).toHaveLength(1);

expect(appModule.methods[0].name).toBe('quit');

expect(appModule.properties).toHaveLength(1);

expect(appModule.properties[0].name).toBe('name');

The type assertions can be removed by using assert from vitest and letting tsc infer types. The line assert(appModule?.type === 'Module') will automatically infer ModuleDocumentationContainer for the following lines and we don't need to use type assertions.

dsanders11 · 2025-11-23T22:49:09Z

tests/DocsParser.spec.ts

+      const menuClass = result.find((c) => c.name === 'Menu');
+      expect(menuClass).toBeDefined();
+
+      const cls = menuClass as ClassDocumentationContainer;
+      expect(cls.staticMethods).toHaveLength(1);
+      expect(cls.staticMethods[0].name).toBe('buildFromTemplate');
+      expect(cls.staticMethods[0].returns).toBeDefined();
+
+      expect(cls.staticProperties).toHaveLength(1);
+      expect(cls.staticProperties[0].name).toBe('applicationMenu');


Suggested change

const menuClass = result.find((c) => c.name === 'Menu');

expect(menuClass).toBeDefined();

const cls = menuClass as ClassDocumentationContainer;

expect(cls.staticMethods).toHaveLength(1);

expect(cls.staticMethods[0].name).toBe('buildFromTemplate');

expect(cls.staticMethods[0].returns).toBeDefined();

expect(cls.staticProperties).toHaveLength(1);

expect(cls.staticProperties[0].name).toBe('applicationMenu');

const menuClass = result.find((c) => c.name === 'Menu');

assert(menuClass?.type === 'Class');

expect(menuClass.staticMethods).toHaveLength(1);

expect(menuClass.staticMethods[0].name).toBe('buildFromTemplate');

expect(menuClass.staticMethods[0].returns).toBeDefined();

expect(menuClass.staticProperties).toHaveLength(1);

expect(menuClass.staticProperties[0].name).toBe('applicationMenu');

Same as above, just another example of the assert removing the need for a type assertion.

Does assert have a useful error message? Sorry the reason I've not used assert in the past is because the error is normally. "Expected true to be false" which isn't helpful

assert takes a second optional argument for message, so you can easily set a more useful error message. Just tested, without one set, if the assert fails you get a somewhat ugly object dump, but in both cases, Vitest shows you the exact line which failed, so you can see the condition easily. Passing a message does make for a nicer experience, so let's do that. 👍

…zfwXb

…age-0189ruZ2dWxVS4KVRFozfwXb

socket-security · 2026-03-24T09:33:13Z

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action	Severity	Alert (click "▶" to expand/collapse)
Warn		High CVE: npm `minimatch` has ReDoS: matchOne() combinatorial backtracking via multiple non-adjacent GLOBSTAR segments CVE: GHSA-7r86-cg39-jmmj minimatch has ReDoS: matchOne() combinatorial backtracking via multiple non-adjacent GLOBSTAR segments (HIGH) Affected versions: >= 10.0.0 < 10.2.3; >= 9.0.0 < 9.0.7; >= 8.0.0 < 8.0.6; >= 7.0.0 < 7.4.8; >= 6.0.0 < 6.2.2; >= 5.0.0 < 5.1.8; >= 4.0.0 < 4.2.5; < 3.1.3 Patched version: 9.0.7 From: `?` → `npm/vitest@4.0.18` → `npm/@vitest/coverage-v8@3.0.5` → `npm/minimatch@9.0.5` ℹ Read more on: This package \| This alert \| What is a CVE? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Remove or replace dependencies that include known high severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/minimatch@9.0.5`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		High CVE: npm `minimatch` ReDoS: nested () extglobs generate catastrophically backtracking regular expressions CVE:* GHSA-23c5-xmqv-rm74 minimatch ReDoS: nested () extglobs generate catastrophically backtracking regular expressions (HIGH) Affected versions:* >= 10.0.0 < 10.2.3; >= 9.0.0 < 9.0.7; >= 8.0.0 < 8.0.6; >= 7.0.0 < 7.4.8; >= 6.0.0 < 6.2.2; >= 5.0.0 < 5.1.8; >= 4.0.0 < 4.2.5; < 3.1.4 Patched version: 9.0.7 From: `?` → `npm/vitest@4.0.18` → `npm/@vitest/coverage-v8@3.0.5` → `npm/minimatch@9.0.5` ℹ Read more on: This package \| This alert \| What is a CVE? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Remove or replace dependencies that include known high severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/minimatch@9.0.5`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		High CVE: npm `minimatch` has a ReDoS via repeated wildcards with non-matching literal in pattern CVE: GHSA-3ppc-4f35-3m26 minimatch has a ReDoS via repeated wildcards with non-matching literal in pattern (HIGH) Affected versions: >= 10.0.0 < 10.2.1; >= 9.0.0 < 9.0.6; >= 8.0.0 < 8.0.5; >= 7.0.0 < 7.4.7; >= 6.0.0 < 6.2.1; >= 5.0.0 < 5.1.7; >= 4.0.0 < 4.2.4; < 3.1.3 Patched version: 9.0.6 From: `?` → `npm/vitest@4.0.18` → `npm/@vitest/coverage-v8@3.0.5` → `npm/minimatch@9.0.5` ℹ Read more on: This package \| This alert \| What is a CVE? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Remove or replace dependencies that include known high severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/minimatch@9.0.5`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Deprecated by its maintainer: npm `tar` Reason: Old versions of tar are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me From: `?` → `npm/vitest@4.0.18` → `npm/tar@7.5.2` ℹ Read more on: This package \| This alert \| What is a deprecated package? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/tar@7.5.2`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

MarshallOfSound requested a review from a team as a code owner November 15, 2025 07:46

MarshallOfSound changed the title ~~spec: add a bunch of tests~~ test: add a bunch of tests Nov 15, 2025

dsanders11 requested changes Nov 23, 2025

View reviewed changes

claude and others added 5 commits November 23, 2025 12:35

chore: more tests

880b209

fix typescript

3ef5c06

use type assertions

a67e85c

MarshallOfSound force-pushed the claude/add-test-coverage-0189ruZ2dWxVS4KVRFozfwXb branch from a5c756d to a67e85c Compare November 23, 2025 20:45

dsanders11 requested changes Nov 23, 2025

View reviewed changes

dsanders11 added 4 commits December 3, 2025 15:01

chore: update yarn.lock

5df4d1f

test: use assert instead of type assertions

aa38fda

chore: update yarn.lock

63a26ba

Merge branch 'main' into claude/add-test-coverage-0189ruZ2dWxVS4KVRFo…

e0b2117

…zfwXb

dsanders11 approved these changes Dec 3, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into claude/add-test-cover…

d7bc8ac

…age-0189ruZ2dWxVS4KVRFozfwXb

MarshallOfSound merged commit e43e170 into main Mar 24, 2026
7 checks passed

MarshallOfSound deleted the claude/add-test-coverage-0189ruZ2dWxVS4KVRFozfwXb branch March 24, 2026 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add a bunch of tests#157

test: add a bunch of tests#157
MarshallOfSound merged 10 commits intomainfrom
claude/add-test-coverage-0189ruZ2dWxVS4KVRFozfwXb

MarshallOfSound commented Nov 15, 2025

Uh oh!

socket-security bot commented Nov 15, 2025 •

edited

Loading

Uh oh!

dsanders11 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarshallOfSound commented Nov 23, 2025

Uh oh!

dsanders11 left a comment

Uh oh!

dsanders11 Nov 23, 2025

Uh oh!

dsanders11 Nov 23, 2025

Uh oh!

MarshallOfSound Nov 25, 2025

Uh oh!

dsanders11 Nov 25, 2025

Uh oh!

socket-security bot commented Mar 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

MarshallOfSound commented Nov 15, 2025

Uh oh!

socket-security bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsanders11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarshallOfSound commented Nov 23, 2025

Uh oh!

dsanders11 left a comment

Choose a reason for hiding this comment

Uh oh!

dsanders11 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

dsanders11 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

dsanders11 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

socket-security bot commented Mar 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

socket-security bot commented Nov 15, 2025 •

edited

Loading