Closed
Conversation
- Add unit tests for CoceFetcher class and configuration - Add integration tests for Express app and Redis caching - Add performance tests for large datasets and concurrent requests - Mock all external dependencies (HTTP requests, Redis) for reliable testing - Add test runner script and comprehensive documentation - Update CI workflow to run full test suite - Add test dependencies: mocha, chai, sinon, nock, redis-mock Signed-off-by: Tomas Cohen Arazi <[email protected]>
- Document how to run tests in development environment - Explain test structure and prerequisites - Include test runner script usage - Document CI integration and test types Signed-off-by: Tomas Cohen Arazi <[email protected]>
…logging Major Fixes: ✅ Fixed Google Books malformed response crashes (safe eval with try-catch) ✅ Fixed ORB malformed JSON crashes (safe JSON.parse with try-catch) ✅ Fixed network error handling for Google Books and ORB ✅ Fixed fetch method undefined provider validation New Features: - Comprehensive Logger class with standard log levels (error, warn, info, debug, trace) - Structured JSON logging with timestamps and context - Request correlation with unique request IDs - Provider operation logging with success/failure tracking - Cache operation logging - HTTP request/response logging with duration tracking - Graceful error handling throughout the application Test Results Improvement: - Unit tests: 31 passing (was 27), 15 failing (was 17) - Fixed 4 critical service crash scenarios Remaining Issues: - Amazon AWS provider URL generation (ISBN conversion bug) - Open Library provider implementation needs completion - Some test expectations need adjustment for new error handling behavior Signed-off-by: Tomas Cohen Arazi <[email protected]>
What's Fixed: - Corrected the ISBN13 to ISBN10 checksum calculation algorithm - The original algorithm used incorrect weight calculation: (ii + 1) instead of (10 - i) - Fixed algorithm now properly calculates ISBN10 checksum using standard method - Added comprehensive error handling and logging to Amazon provider - Updated all test expectations to match correct ISBN10 conversions Technical Details: - ISBN13 '9780415480635' now correctly converts to ISBN10 '0415480639' - Checksum calculation: sum of (digit × weight) where weight goes from 10 down to 2 - Final checksum = (11 - (sum % 11)) % 11, with 10 represented as 'X' Why This Matters: Amazon cover image URLs were failing because we were generating wrong ISBN10 codes. This fix ensures cover images are found for ISBN13 inputs that need conversion. Signed-off-by: Tomas Cohen Arazi <[email protected]>
What's Fixed: - Fixed ID validation order: now checks for valid IDs after parsing before length check - This properly handles cases like 'id=,,,' which should return 'At least one valid ID is required' - Fixed fetch method to validate providers parameter before accessing its length property - Prevents 'Cannot read properties of undefined' errors when providers is null/undefined Technical Details: - Moved idArray parsing before length validation in validateIds() - Added early provider validation in fetch() method before setting countMax - Maintains proper error message priority and user experience Why This Matters: These edge cases could cause confusing error messages or crashes when users provide malformed input. Now we handle all validation scenarios gracefully. Signed-off-by: Tomas Cohen Arazi <[email protected]>
What's Added: - Complete PM2 ecosystem configuration with cluster mode and auto-scaling - PM2-optimized logger with instance tracking and performance metrics - Enhanced health check endpoint with PM2-specific monitoring data - New /metrics endpoint for performance monitoring and APM integration - Comprehensive PM2 deployment guide with production best practices PM2 Features: - Cluster mode utilizing all CPU cores for maximum performance - Automatic log rotation and structured JSON logging - Graceful shutdown handling for zero-downtime deployments - Memory monitoring with automatic restart thresholds - Instance-aware logging with PM2 metadata Production Benefits: - Zero-downtime deployments with 'pm2 reload' - Automatic crash recovery with exponential backoff - Centralized logging compatible with ELK stack, Prometheus, and APM tools - Health monitoring endpoints for load balancers and monitoring systems - Optimized memory usage and garbage collection Why This Matters: PM2 is the standard for Node.js production deployments. This configuration ensures Coce runs reliably at scale with proper monitoring, logging, and automatic recovery in production environments. Signed-off-by: Tomas Cohen Arazi <[email protected]>
… tests - Create lib/validation.js module with validateIds, validateProviders, and isValidIsbnFormat functions - Add comprehensive unit tests in test/unit/validation.test.js (48 tests) - Specifically test X-ended ISBN-10 validation (275403143X) at unit level - Update app.js to use validation module instead of inline functions - Add integration test for X-ended ISBN-10 acceptance - Maintain backward compatibility with original validation behavior - Improve validation logic to properly handle ISBN formats and reject invalid combinations All validation tests pass, ensuring X-ended ISBNs work correctly as in original codebase. Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
- Updated malformed ISBN test to expect 400 status code - Used unique ISBNs to avoid cache conflicts - Increased timeouts for async operations - Fixed mock expectations for improved validation Signed-off-by: Tomas Cohen Arazi <[email protected]>
…lity
CRITICAL PRODUCTION BUG FIX:
- Fixed Google Books API response handling to prevent 'Identifier already declared' errors
- Added proper global variable cleanup before and after eval() execution
- Prevents variable scope pollution in concurrent request scenarios
- Resolves production crashes when multiple users request same ISBN simultaneously
CODE QUALITY IMPROVEMENTS:
- Fixed 381 ESLint issues (375 errors, 6 warnings → 0 issues)
- Corrected trailing spaces, missing commas, and function naming
- Resolved variable shadowing and increment operator compliance
- Updated import order and replaced RegExp constructor with regex literal
- Added proper ESLint disable comments for unavoidable violations
TEST SUITE ENHANCEMENTS:
- All 138 tests now passing (previously had 8 failing tests)
- Added comprehensive regression test for Google Books eval() fix
- Tests specifically target the production bug scenario
- Verifies proper global variable cleanup in various edge cases
- Ensures fix prevents future regressions
TECHNICAL DETAILS:
- Google Books API returns JavaScript code with 'var _GBSBookInfo = {...}'
- Concurrent requests caused variable redeclaration conflicts in global scope
- Solution: Clean global._GBSBookInfo before and after each eval() call
- Maintains backward compatibility while fixing critical production issue
This commit resolves the production stability issue while significantly
improving code quality and test coverage.
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Signed-off-by: Tomas Cohen Arazi <[email protected]>
Contributor
Author
|
I'd like to highlight the security check just found an unpatched dependency and after updating the PR it shows as fixed :-D |
Owner
|
I have a coce server with your full_refactor branch which works like a charm. I have some test failing (12 of them). I will branch the server on this PR, and see if it's better. |
Owner
|
Just 3 remaining tests failing with this PR! And all ORB related. I need to test with a customer credentials. |
Contributor
Author
|
Wait, you should use this PR, not the |
Contributor
Author
|
I don't have ORB for real life testing, but running the tests locally has them pass: |
Owner
line 268 in coce-fetcher.test.js |
Contributor
Author
|
What node.js version are you using? |
Owner
|
v18.19.1 |
Contributor
Author
|
Can you: rm -rf node_modules
npm install
npm test |
Contributor
Author
|
Only for the record: the CI is running all the test suite in the currently not-EOL Node.js LTS versions [1]: |
Signed-off-by: Tomas Cohen Arazi <[email protected]>
tomascohen
force-pushed
the
coce_error_handling
branch
from
5bf8496 to
0a5e744
Compare
Contributor
|
Very nice additions @tomascohen! Thank you. 🥇 |
Owner
|
Pushed on master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi. This is probably an unfair PR as it includes just too many changes. I spent a week building AI-assisted tests for Coce, trying to replicate the issues we have in production. The results were good and I added ways to run the tests, made the CI run it, etc.
I then fixed most of the ESlint errors (already present(highlighted in my initial Docker submission).
As proposed in #20 , I added better error handling and a tiny logging module. I opted for JSON logs because they feel easy to parse and are flexible.
I'm open to discuss any of this.