-
Notifications
You must be signed in to change notification settings - Fork 53
MLE-24722 - make sure the done() method called within each test once … #985
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
MLE-24722 - make sure the done() method called within each test once … #985
Conversation
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
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 addresses test stability issues by ensuring the done() method is called exactly once in each test and implementing reasonable timeouts for bulk operations. The changes focus on preventing tests from hanging by removing infinite timeouts and adding proper error handling.
- Replaces infinite timeouts (
this.timeout(0)) with reasonable 30-second and 120-second timeouts - Adds proper error handling with try-catch blocks to ensure
done()is called once - Ensures tests that expect errors call
done()with appropriate error messages when exceptions don't occur
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test-basic/service-caller.js | Adds 30-second timeout for test setup |
| test-basic/endpoint-caller.js | Adds 30-second timeout for test setup |
| test-basic/documents-data-movement-readAll.js | Replaces infinite timeout with 120-second timeout and updates comments |
| test-basic/documents-data-movement-queryAll.js | Replaces infinite timeout with 120-second timeout and updates comments |
| test-basic/documents-core.js | Adds try-catch blocks around assertions to ensure proper error handling |
| test-basic/docColTypes-test.js | Ensures error is thrown when invalid document descriptor is used |
| test-basic/digestauth-fips-nomd5load.js | Adds missing done parameter and proper error handling |
| test-basic/cloud_authentication-test.js | Ensures error is thrown when apiKey is missing |
| test-basic/client.js | Adds try-catch blocks and ensures errors are thrown for invalid configurations |
| test-basic/bindingFromParam.js | Adds proper error handling and ensures expected errors are thrown |
| test-basic/basePath-test.js | Ensures errors are thrown for invalid basePath configurations |
| test-basic/annTopK.js | Ensures error is thrown for invalid options argument |
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.
Looks good, just adding comments I got from Copilot locally.
…and only once. Set reasonable timeouts for some of the bulk operations and remove infinite timeouts which can hang the test.
02ff571 to
d9a236a
Compare
…ations. Change annTopK tests to use await/rejects idiom which is easier to understand. Skip the cloud test that tries to connect to support.beta.marklogic.cloud. That DNS name is not resolving as of today.
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.
I just forced the pipeline to run again to see if we get a green build. I'm sure the 4 failures aren't related to this because they're all "test-complete" tests. But those failures also show at least one test where "done is called multiple times", thus masking the real failure.
…and only once. Set reasonable timeouts for some of the bulk operations and remove infinite timeouts which can hang the test.