Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 27, 2025
Merged

fix: don't execute cleanup code if setup failed #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7971332
fix: don't execute cleanup code if setup failed
nirinchev Aug 27, 2025
4b67d43
add more oidc test retry conditions
nirinchev Aug 27, 2025
2c30606
give it more time
nirinchev Aug 27, 2025
3e65705
Merge branch 'main' into ni/atlas-tests-guards
nirinchev Aug 27, 2025
ef45fdb
add exportTitle matchers to accuracy tests
nirinchev Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions tests/accuracy/export.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describeAccuracyTests([
parameters: {
database: "mflix",
collection: "movies",
exportTitle: Matcher.string(),
Copy link
Collaborator Author

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.

exportTarget: [
{
name: "find",
Expand All @@ -28,6 +29,7 @@ describeAccuracyTests([
parameters: {
database: "mflix",
collection: "movies",
exportTitle: Matcher.string(),
exportTarget: [
{
name: "find",
Expand All @@ -50,6 +52,7 @@ describeAccuracyTests([
parameters: {
database: "mflix",
collection: "movies",
exportTitle: Matcher.string(),
exportTarget: [
{
name: "find",
Expand Down Expand Up @@ -77,6 +80,7 @@ describeAccuracyTests([
parameters: {
database: "mflix",
collection: "movies",
exportTitle: Matcher.string(),
exportTarget: [
{
name: "find",
Expand Down
33 changes: 22 additions & 11 deletions tests/integration/common/connectionManager.oidc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,28 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
};
};

const status: ConnectionStatus = await vi.waitFor(async () => {
const result: ConnectionStatus = (await state.serviceProvider.runCommand("admin", {
connectionStatus: 1,
})) as unknown as ConnectionStatus;

if (!result) {
throw new Error("Status can not be undefined. Retrying.");
}

return result;
});
const status: ConnectionStatus = await vi.waitFor(
async () => {
const result = (await state.serviceProvider.runCommand("admin", {
connectionStatus: 1,
})) as unknown as ConnectionStatus | undefined;

if (!result) {
throw new Error("Status can not be undefined. Retrying.");
}

if (!result.authInfo.authenticatedUsers.length) {
throw new Error("No authenticated users found. Retrying.");
}

if (!result.authInfo.authenticatedUserRoles.length) {
throw new Error("No authenticated user roles found. Retrying.");
}

return result;
},
{ timeout: 5000 }
Copy link
Collaborator Author

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

);

expect(status.authInfo.authenticatedUsers[0]).toEqual({
user: "dev/testuser",
Expand Down
20 changes: 11 additions & 9 deletions tests/integration/tools/atlas/accessLists.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,18 @@ describeWithAtlas("ip access lists", (integration) => {
const apiClient = integration.mcpServer().session.apiClient;

const projectId = getProjectId();

for (const value of values) {
await apiClient.deleteProjectIpAccessList({
params: {
path: {
groupId: projectId,
entryValue: value,
if (projectId) {
// projectId may be empty if beforeAll failed.
for (const value of values) {
await apiClient.deleteProjectIpAccessList({
params: {
path: {
groupId: projectId,
entryValue: value,
},
},
},
});
});
}
}
});

Expand Down
22 changes: 12 additions & 10 deletions tests/integration/tools/atlas/atlasHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export function withProject(integration: IntegrationTest, fn: ProjectTestFunctio

try {
const group = await createProject(apiClient);
projectId = group.id || "";
projectId = group.id;
} catch (error) {
console.error("Failed to create project:", error);
throw error;
Expand All @@ -50,14 +50,16 @@ export function withProject(integration: IntegrationTest, fn: ProjectTestFunctio

afterAll(async () => {
const apiClient = integration.mcpServer().session.apiClient;

await apiClient.deleteProject({
params: {
path: {
groupId: projectId,
if (projectId) {
// projectId may be empty if beforeAll failed.
await apiClient.deleteProject({
params: {
path: {
groupId: projectId,
},
},
},
});
});
}
});

const args = {
Expand Down Expand Up @@ -90,7 +92,7 @@ export function parseTable(text: string): Record<string, string>[] {

export const randomId = new ObjectId().toString();

async function createProject(apiClient: ApiClient): Promise<Group> {
async function createProject(apiClient: ApiClient): Promise<Group & Required<Pick<Group, "id">>> {
const projectName: string = `testProj-` + randomId;

const orgs = await apiClient.listOrganizations();
Expand All @@ -109,5 +111,5 @@ async function createProject(apiClient: ApiClient): Promise<Group> {
throw new Error("Failed to create project");
}

return group;
return group as Group & Required<Pick<Group, "id">>;
}
8 changes: 4 additions & 4 deletions tests/integration/tools/atlas/clusters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ describeWithAtlas("clusters", (integration) => {

afterAll(async () => {
const projectId = getProjectId();

const session: Session = integration.mcpServer().session;

await deleteAndWaitCluster(session, projectId, clusterName);
if (projectId) {
const session: Session = integration.mcpServer().session;
await deleteAndWaitCluster(session, projectId, clusterName);
}
});

describe("atlas-create-free-cluster", () => {
Expand Down
8 changes: 7 additions & 1 deletion tests/integration/tools/atlas/dbUsers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,17 @@ describeWithAtlas("db users", (integration) => {
};

afterEach(async () => {
const projectId = getProjectId();
if (!projectId) {
// projectId may be empty if beforeAll failed
return;
}

try {
await integration.mcpServer().session.apiClient.deleteDatabaseUser({
params: {
path: {
groupId: getProjectId(),
groupId: projectId,
username: userName,
databaseName: "admin",
},
Expand Down
Loading