-
Notifications
You must be signed in to change notification settings - Fork 111
fix: don't execute cleanup code if setup failed #483
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
Conversation
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Pull Request Test Coverage Report for Build 17267759772Details
💛 - Coveralls |
@@ -10,6 +10,7 @@ describeAccuracyTests([ | |||
parameters: { | |||
database: "mflix", | |||
collection: "movies", | |||
exportTitle: Matcher.string(), |
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.
cc @himanshusinghs - not sure what has happened here - I think I added the titles at some point, then they disappeared 😬 But looking at the results on main, it seems the accuracy tests are failing because of this.
|
||
return result; | ||
}, | ||
{ timeout: 5000 } |
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.
cc @kmruiz - I expanded this a bit as the test was failing due to the authenticatedUsers
collection being empty, not status
itself being undefined. I also added it a bit more time since I saw it fail with the 1 second default.
Here's a run where we're only checking status: https://github.com/mongodb-js/mongodb-mcp-server/actions/runs/17266097331/job/48998448665?pr=483
Here's the one that's failing with the 1 second default: https://github.com/mongodb-js/mongodb-mcp-server/actions/runs/17267312116/job/49002419896?pr=483
📊 Accuracy Test Results📈 Summary
📎 Download Full HTML Report - Look for the Report generated on: 8/27/2025, 1:21:02 PM |
Proposed changes
The atlas tests would try and cleanup some resources in afterAll/afterEach, but those would run even if beforeAll failed. This was causing some red herring errors due to calling atlas endpoints with an empty projectId, for example. While this doesn't address the underlying problems that caused the setup to fail, at least it reduces the noise.