Skip to content

Conversation

@taralamia
Copy link
Contributor

What's in this PR?

This PR adds comprehensive unit tests for the OAuthError and TimeoutError classes in src/errors.ts, addressing issue #3.

What was tested?

The tests cover all requirements from the issue:
✅ Constructor property assignment
✅ Error inheritance from native Error class
✅ Default message fallbacks
✅ Correct error naming (OAuthError, TimeoutError)
✅ Stack trace maintenance
✅ JSON serialization behavior
✅ Edge cases (long strings, unicode, null/undefined values)
✅ Error comparison (instance vs. value equality)
✅ Error recovery (verified no implementation in current design)

Key Findings (Please Read)

All tests pass except for the JSON serialization test. This test reveals the current behavior:

  • OAuthError: Only the custom properties (error, error_description, error_uri) are serialized. The name and message properties from the inherited Error class are omitted because they are non-enumerable.
  • TimeoutError: Serializes to an empty object {} for the same reason.
    This is the current, tested behavior. If this is not the desired behavior for the library (e.g., if errors should serialize fully for logging), a fix would require implementing a toJSON() method on these classes. This could be explored in a future issue.

Notes

  • I followed the suggested test structure using bun:test.
  • The tests confirm the core error-handling logic is solid and works as intended.
  • Utility functions were created for robust error comparison testing (sameErrorInstance, equalErrorByValue, equalErrorStrict).

taralamia and others added 5 commits August 22, 2025 03:09
- Fixed critical bug where TimeoutError test was using OAuthError class
- Removed duplicate stack trace tests (lines 63-68)
- Restructured tests with nested describe blocks for better organization
- Added 13 new tests covering JSON serialization, edge cases, and error comparison
- Enhanced test coverage from 10 to 23 tests with 60 assertions
- Removed package-lock.json and added to .gitignore (Bun project uses bun.lock)
- Cleaned up inconsistent inline comments in favor of descriptive test names

All tests passing successfully. Ready for merge.
@koistya
Copy link
Member

koistya commented Aug 22, 2025

Hey @taralamia! Thanks for getting this PR started. I've pushed some improvements to help get it ready for merge:

Fixed issues:

  • Line 64 was creating the wrong error type (OAuthError instead of TimeoutError) - fixed ✅
  • Removed the duplicate stack trace test that was causing confusion
  • Cleaned up the NPM lock file that snuck in (we're using Bun)

Enhanced the tests:

  • Expanded from 10 to 23 tests with much better coverage
  • Added the missing JSON serialization tests you mentioned
  • Included edge cases for empty strings, long strings, and unicode
  • Added error comparison tests to verify instance uniqueness
  • Reorganized everything with nested describe blocks for clarity

All tests are passing now! The coverage looks solid and addresses everything from issue #3. Nice work on the initial implementation - the foundation was good, just needed some polish.

Let me know if you want to review any of the changes!

@koistya koistya changed the title Test/errors: add comprehensive unit tests for OAuthError and TimeoutError Add comprehensive unit tests for error classes Aug 22, 2025
@koistya koistya merged commit ce9af5e into kriasoft:main Aug 22, 2025
6 checks passed
@taralamia taralamia deleted the test/errors branch August 22, 2025 17:13
@taralamia
Copy link
Contributor Author

Thanks @koistya I really appreciate you polishing up the PR and adding those comprehensive tests. And I’m happy to contribute since I got to learn more about Bun in the process.

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