chore: reduce cleanup time #652
Conversation
blva
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR reduces cleanup time for test resources and enhances test organization by creating a more flexible credential handling system. It changes the cleanup threshold from 24 hours to 2 hours to prevent test resource accumulation.
- Reduces cleanup time from 24 hours to 2 hours for Atlas test resources
- Adds credential validation wrapper for Atlas integration tests
- Improves null safety in cleanup script
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/integration/tools/atlas/orgs.test.ts | Wraps tests with credential validation using new withCredentials helper |
| tests/integration/tools/atlas/atlasHelpers.ts | Adds withCredentials function and fallback credentials for test configuration |
| scripts/cleanupAtlasTestLeftovers.test.ts | Reduces cleanup time to 2 hours and adds null safety check for organization ID |
| apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client", | ||
| apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret", |
There was a problem hiding this comment.
Using hardcoded fallback credentials ('test-client', 'test-secret') creates a security risk. These fallback values should be removed or replaced with proper test environment validation that fails gracefully when credentials are missing.
| apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client", | |
| apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret", | |
| apiClientId: process.env.MDB_MCP_API_CLIENT_ID, | |
| apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I just noticed this, why don't we use defaultTestConfig here which ideally should already parse these values?
There was a problem hiding this comment.
we use defaultTestConfig but override it with the test items if it's not there
|
|
||
| export function withCredentials(integration: IntegrationTest, fn: IntegrationTestFunction): SuiteCollector<object> { | ||
| const describeFn = | ||
| !process.env.MDB_MCP_API_CLIENT_ID?.length || !process.env.MDB_MCP_API_CLIENT_SECRET?.length |
There was a problem hiding this comment.
The credential validation logic is duplicated between describeWithAtlas and withCredentials. Consider extracting this into a shared helper function to reduce code duplication.
Copilot uses AI. Check for mistakes.
| apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client", | ||
| apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret", |
There was a problem hiding this comment.
I just noticed this, why don't we use defaultTestConfig here which ideally should already parse these values?
3 participants
Proposed changes
Checklist