-
Notifications
You must be signed in to change notification settings - Fork 17
Add vitest and unit tests for existing functionality #123
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.
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
base: main
Are you sure you want to change the base?
Conversation
eedb701
to
2366bd4
Compare
2366bd4
to
f2e8b5a
Compare
f2e8b5a
to
04a3506
Compare
04a3506
to
e785952
Compare
There was a problem hiding this 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.
There was a problem hiding this 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>; |
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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>; |
There was a problem hiding this comment.
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
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
yarn
to update yournode_modules
yarn test:coverage
to run the testsExample