MLE-24194 Fixing checkConnection and better tests#1036
Conversation
rjrudin
requested review from
BillFarber,
anu3990 and
stevebio
as code owners
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug in the checkConnection method's TypeScript return type and adds comprehensive runtime testing. The method was incorrectly typed to return ConnectionCheckResult directly instead of ResultProvider<ConnectionCheckResult>, causing it to fail in actual TypeScript code that needed to call .result() to get the promise.
Key changes:
- Fixed TypeScript definition for
checkConnectionto returnResultProvider<ConnectionCheckResult> - Added runtime test suite to validate TypeScript definitions match actual JavaScript behavior
- Updated test infrastructure to support both compile-only and runtime TypeScript tests
Reviewed Changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test-typescript/checkConnection-runtime.test.ts | New comprehensive runtime test validating checkConnection returns ResultProvider with proper type safety |
| test-typescript/README.md | Enhanced documentation explaining the dual approach of compile-only vs runtime TypeScript tests |
| typescript-test-project/test.ts | Simplified to realistic usage pattern, correctly calling .result() on checkConnection |
| typescript-test-project/tsconfig.json | Enabled compilation output for runtime execution of the test project |
| typescript-test-project/package.json | Added scripts to build and run the TypeScript test project |
| package.json | Added test:compile script and @types/mocha dependency for runtime TypeScript tests |
| lib/responder.js | Added null check for connectionParams before accessing apiKey property |
| Jenkinsfile | Integrated new TypeScript runtime tests into CI pipeline across all MarkLogic versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
package.json
Outdated
| "lint": "gulp lint", | ||
| "test:types": "tsc --noEmit" | ||
| "test:types": "tsc --noEmit", | ||
| "test:compile": "tsc test-typescript/checkConnection-runtime.test.ts --outDir test-typescript --module commonjs --target ES2020 --esModuleInterop --skipLibCheck", |
There was a problem hiding this comment.
The long inline TypeScript compilation command is difficult to maintain. Consider moving these compiler options to a dedicated tsconfig file (e.g., test-typescript/tsconfig.json) and simplifying the script to 'tsc -p test-typescript', which would be more maintainable and consistent with standard TypeScript project structure.
| "test:compile": "tsc test-typescript/checkConnection-runtime.test.ts --outDir test-typescript --module commonjs --target ES2020 --esModuleInterop --skipLibCheck", | |
| "test:compile": "tsc -p test-typescript", |
rjrudin
force-pushed
the
feature/testing-3
branch
2 times, most recently
from
d990dd4 to
243e6ba
Compare
This was way harder than it should have been, but Copilot finally got it done. checkConnection had the wrong return type, so the method didn't actually working in a "real" TS test - and we now have a "real" TS test. See the Copilot-authored README for explanation.
rjrudin
force-pushed
the
feature/testing-3
branch
from
243e6ba to
77a1a2c
Compare
1 participant
This was way harder than it should have been, but Copilot finally got it done. checkConnection had the wrong return type, so the method didn't actually working in a "real" TS test - and we now have a "real" TS test. See the Copilot-authored README for explanation.