test: add a bunch of tests #157

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add a bunch of tests #157

test: add a bunch of tests #157

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

test: add a bunch of tests #157

Are you sure you want to change the base?

test: add a bunch of tests #157

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

socket-security bot commented Nov 15, 2025 •

edited

Loading