Skip to content

Conversation

jamaljsr
Copy link
Member

@jamaljsr jamaljsr commented Sep 10, 2025

In preparation for some upcoming improvements to this library, I have added many unit tests to ensure there are no breaking changes to the existing functionality going forward. I chose to use the Vitest testing framework over Jest because it is substantially faster. I have also added a Github workflow job to run the tests in CI.

Steps to Test

  1. Run yarn to update your node_modules
  2. Run yarn test:coverage to run the tests
  3. Confirm all the tests pass

Example

$ yarn test:coverage
yarn run v1.22.17
$ vitest run --coverage

 RUN  v3.2.4 /Users/jamal/dev/lnc-web
      Coverage enabled with v8

 ✓ lib/util/log.test.ts (20 tests) 6ms
 ✓ lib/api/createRpc.test.ts (9 tests) 4ms
 ✓ lib/util/credentialStore.test.ts (27 tests) 15ms
 ✓ lib/util/encryption.test.ts (19 tests) 17ms
 ✓ lib/index.test.ts (2 tests) 59ms
 ✓ lib/lnc.test.ts (52 tests) 98ms

 Test Files  6 passed (6)
      Tests  129 passed (129)
   Start at  08:02:38
   Duration  468ms (transform 229ms, setup 122ms, collect 397ms, tests 198ms, environment 1ms, prepare 258ms)

 % Coverage report from v8
---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------------|---------|----------|---------|---------|-------------------
All files            |   99.37 |    96.26 |   98.14 |   99.37 |                   
 lib                 |   98.74 |    94.91 |      95 |   98.74 |                   
  index.ts           |      70 |      100 |       0 |      70 | 9-11              
  lnc.ts             |     100 |    94.91 |     100 |     100 | 62,203            
 lib/api             |     100 |      100 |     100 |     100 |                   
  createRpc.ts       |     100 |      100 |     100 |     100 |                   
 lib/types           |       0 |        0 |       0 |       0 |                   
  lnc.ts             |       0 |        0 |       0 |       0 |                   
 lib/util            |     100 |    97.01 |     100 |     100 |                   
  credentialStore.ts |     100 |    95.34 |     100 |     100 | 221,232           
  encryption.ts      |     100 |      100 |     100 |     100 |                   
  log.ts             |     100 |      100 |     100 |     100 |                   
---------------------|---------|----------|---------|---------|-------------------
✨  Done in 0.94s.

@jamaljsr jamaljsr self-assigned this Sep 10, 2025
@jamaljsr jamaljsr force-pushed the add-unit-tests branch 2 times, most recently from eedb701 to 2366bd4 Compare September 10, 2025 13:31
@jamaljsr jamaljsr requested a review from Copilot September 10, 2025 13:32
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

Copy link

@Copilot 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

This PR adds comprehensive unit test coverage to the Lightning Node Connect (LNC) web library using Vitest as the testing framework. The purpose is to establish test coverage for existing functionality to prevent breaking changes during future improvements.

  • Replaces Jest/Mocha with Vitest for faster test execution
  • Adds extensive test coverage across all core modules with 129 passing tests
  • Integrates GitHub Actions workflow for automated CI testing

Reviewed Changes

Copilot reviewed 19 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vitest.config.ts Configuration for Vitest test runner with coverage settings
package.json Updates test scripts and dependencies to use Vitest
test/setup.ts Global test setup with mocks for browser APIs and WebAssembly
test/mocks/* Mock implementations for localStorage, WebAssembly, and test utilities
test/utils/* Helper utilities and factories for test data and mock setup
lib/**/*.test.ts Comprehensive unit tests for all core modules
lib/lnc.ts Minor refactoring to use typed global references
lib/util/log.ts Added getter for log level property
tslint.json Removed unused variable rule
.prettierignore Added coverage directory exclusion
.github/workflows/test.yml GitHub Actions workflow for running tests in CI

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@jbrill jbrill left a comment

Choose a reason for hiding this comment

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

Very impressive coverage 💪

Don't see any negatives. The tests look solid 🙏

Nit: we could have the coverage summary a little more compact for readability if we wanted:

 --reporter=text-summary

const mockLnc = {
request: vi.fn(),
subscribe: vi.fn()
} as unknown as Mocked<LNC>;
Copy link

Choose a reason for hiding this comment

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

do we need the as unknown as Mocked<LNC>? Could we instead have it be a partial?

});

describe('Method name capitalization', () => {
it('should capitalize method names correctly', () => {
Copy link

Choose a reason for hiding this comment

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

Seems like a bit of an odd test -- more of a linting job imo

it('should handle empty config object gracefully', () => {
const lnc = new LNC({});

expect(lnc._wasmClientCode).toBe(DEFAULT_CONFIG.wasmClientCode);
Copy link

Choose a reason for hiding this comment

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

Interesting -- didn't think we could access private members in a test

@@ -1 +1,7 @@
declare var global: any;

interface GoInstance {
run(...args: any[]): Promise<void>;
Copy link

Choose a reason for hiding this comment

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

Can we really run the go instance with any[]? I

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.

2 participants