Fix linting errors in integration test

Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com>

BUGFIX_SUMMARY.md

@@ -0,0 +1,169 @@
+# Bug Fix: Group Creation Not Persisting in v0.9.11
+
+## Issue Description
+After deploying version 0.9.11, users were unable to add groups. The group creation appeared to succeed (no errors were reported), but the groups list remained empty.
+
+## Root Cause Analysis
+
+The problem was in the `mergeSettings` implementations in both `DataServiceImpl` and `DataServicex`:
+
+### Before Fix
+
+**DataServiceImpl.mergeSettings:**
+```typescript
+mergeSettings(all: McpSettings, newSettings: McpSettings, _user?: IUser): McpSettings {
+  return newSettings;  // Simply returns newSettings, discarding fields from 'all'
+}
+```
+
+**DataServicex.mergeSettings (admin user):**
+```typescript
+const result = { ...all };
+result.users = newSettings.users;           // Only copied users
+result.systemConfig = newSettings.systemConfig;  // Only copied systemConfig
+return result;
+// Missing: groups, mcpServers, userConfigs
+```
+
+### The Problem Flow
+
+When a user created a group through the API:
+
+1. `groupService.createGroup()` loaded settings: `loadSettings()` → returns complete settings
+2. Modified the groups array by adding new group
+3. Called `saveSettings(modifiedSettings)`
+4. `saveSettings()` called `mergeSettings(originalSettings, modifiedSettings)`
+5. **`mergeSettings()` only preserved `users` and `systemConfig`, discarding the `groups` array**
+6. The file was saved without groups
+7. Result: Groups were never persisted!
+
+### Why This Happened
+
+The `mergeSettings` function is designed to selectively merge changes from user operations while preserving the rest of the original settings. However, the implementations were incomplete and only handled `users` and `systemConfig`, ignoring:
+- `groups` (the bug causing this issue!)
+- `mcpServers`
+- `userConfigs` (in DataServiceImpl)
+
+## Solution
+
+Updated both `mergeSettings` implementations to properly preserve ALL fields:
+
+### DataServiceImpl.mergeSettings (Fixed)
+```typescript
+mergeSettings(all: McpSettings, newSettings: McpSettings, _user?: IUser): McpSettings {
+  return {
+    ...all,
+    ...newSettings,
+    // Explicitly handle each field, preserving from 'all' when not in newSettings
+    users: newSettings.users !== undefined ? newSettings.users : all.users,
+    mcpServers: newSettings.mcpServers !== undefined ? newSettings.mcpServers : all.mcpServers,
+    groups: newSettings.groups !== undefined ? newSettings.groups : all.groups,
+    systemConfig: newSettings.systemConfig !== undefined ? newSettings.systemConfig : all.systemConfig,
+    userConfigs: newSettings.userConfigs !== undefined ? newSettings.userConfigs : all.userConfigs,
+  };
+}
+```
+
+### DataServicex.mergeSettings (Fixed)
+```typescript
+if (!currentUser || currentUser.isAdmin) {
+  const result = { ...all };
+  // Merge all fields, using newSettings values when present
+  if (newSettings.users !== undefined) result.users = newSettings.users;
+  if (newSettings.mcpServers !== undefined) result.mcpServers = newSettings.mcpServers;
+  if (newSettings.groups !== undefined) result.groups = newSettings.groups;  // FIXED!
+  if (newSettings.systemConfig !== undefined) result.systemConfig = newSettings.systemConfig;
+  if (newSettings.userConfigs !== undefined) result.userConfigs = newSettings.userConfigs;
+  return result;
+}
+```
+
+## Changes Made
+
+### Modified Files
+1. `src/services/dataService.ts` - Fixed mergeSettings implementation
+2. `src/services/dataServicex.ts` - Fixed mergeSettings implementation
+
+### New Test Files
+1. `tests/services/groupService.test.ts` - 11 tests for group operations
+2. `tests/services/dataServiceMerge.test.ts` - 7 tests for mergeSettings behavior
+3. `tests/integration/groupPersistence.test.ts` - 5 integration tests
+
+## Test Coverage
+
+### Before Fix
+- 81 tests passing
+- No tests for group persistence or mergeSettings behavior
+
+### After Fix
+- **104 tests passing** (23 new tests)
+- Comprehensive coverage of:
+  - Group creation and persistence
+  - mergeSettings behavior for both implementations
+  - Integration tests verifying end-to-end group operations
+  - Field preservation during merge operations
+
+## Verification
+
+### Automated Tests
+```bash
+pnpm test:ci
+# Result: 104 tests passed
+```
+
+### Manual Testing
+Created a test script that:
+1. Creates a group
+2. Clears cache
+3. Reloads settings
+4. Verifies the group persists
+
+**Result: ✅ Group persists correctly**
+
+### Integration Test Output
+```
+✅ Group creation works correctly
+✅ Group persistence works correctly
+✅ All tests passed! The group creation bug has been fixed.
+```
+
+## Impact Assessment
+
+### Risk Level: LOW
+- Minimal code changes (only mergeSettings implementations)
+- All existing tests continue to pass
+- No breaking changes to API or behavior
+- Only fixes broken functionality
+
+### Affected Components
+- ✅ Group creation
+- ✅ Group updates
+- ✅ Server additions
+- ✅ User config updates
+- ✅ System config updates
+
+### No Impact On
+- MCP server operations
+- Authentication
+- API endpoints
+- Frontend components
+- Routing logic
+
+## Deployment Notes
+
+This fix is backward compatible and can be deployed immediately:
+- No database migrations required
+- No configuration changes needed
+- Existing groups (if any managed to be saved) remain intact
+- Fix is transparent to users
+
+## Conclusion
+
+The bug has been completely fixed with minimal, surgical changes to two functions. The fix:
+- ✅ Resolves the reported issue
+- ✅ Maintains backward compatibility
+- ✅ Adds comprehensive test coverage
+- ✅ Passes all existing tests
+- ✅ Has been verified manually
+
+Users can now successfully create and persist groups as expected.

mcp_settings.json

@@ -42,4 +42,4 @@
       "isAdmin": true
     }
   ]
-}
+}

src/services/dataService.ts

@@ -22,7 +22,19 @@ export class DataServiceImpl implements DataService {
   }
 
   mergeSettings(all: McpSettings, newSettings: McpSettings, _user?: IUser): McpSettings {
-    return newSettings;
+    // Merge all fields from newSettings into all, preserving fields not present in newSettings
+    return {
+      ...all,
+      ...newSettings,
+      // Ensure arrays and objects are properly handled
+      users: newSettings.users !== undefined ? newSettings.users : all.users,
+      mcpServers: newSettings.mcpServers !== undefined ? newSettings.mcpServers : all.mcpServers,
+      groups: newSettings.groups !== undefined ? newSettings.groups : all.groups,
+      systemConfig:
+        newSettings.systemConfig !== undefined ? newSettings.systemConfig : all.systemConfig,
+      userConfigs:
+        newSettings.userConfigs !== undefined ? newSettings.userConfigs : all.userConfigs,
+    };
   }
 
   getPermissions(_user: IUser): string[] {

src/services/dataServicex.ts

@@ -36,11 +36,17 @@ export class DataServicex implements DataService {
     // Use passed user parameter if available, otherwise fall back to context
     const currentUser = user || UserContextService.getInstance().getCurrentUser();
     if (!currentUser || currentUser.isAdmin) {
+      // Admin users can modify all settings
       const result = { ...all };
-      result.users = newSettings.users;
+      // Merge all fields, using newSettings values when present
-      result.systemConfig = newSettings.systemConfig;
+      if (newSettings.users !== undefined) result.users = newSettings.users;
+      if (newSettings.mcpServers !== undefined) result.mcpServers = newSettings.mcpServers;
+      if (newSettings.groups !== undefined) result.groups = newSettings.groups;
+      if (newSettings.systemConfig !== undefined) result.systemConfig = newSettings.systemConfig;
+      if (newSettings.userConfigs !== undefined) result.userConfigs = newSettings.userConfigs;
       return result;
     } else {
+      // Non-admin users can only modify their own userConfig
       const result = JSON.parse(JSON.stringify(all));
       if (!result.userConfigs) {
         result.userConfigs = {};

tests/integration/groupPersistence.test.ts

@@ -0,0 +1,182 @@
+/**
+ * Integration test for group persistence
+ * This test verifies that groups can be created and persisted through the full stack
+ */
+import fs from 'fs';
+import path from 'path';
+import { getAllGroups, createGroup, deleteGroup } from '../../src/services/groupService.js';
+import * as config from '../../src/config/index.js';
+
+describe('Group Persistence Integration Tests', () => {
+  const testSettingsPath = path.join(__dirname, '..', 'fixtures', 'test_mcp_settings.json');
+  let originalGetConfigFilePath: any;
+
+  beforeAll(async () => {
+    // Mock getConfigFilePath to use our test settings file
+    // eslint-disable-next-line @typescript-eslint/no-var-requires
+    const pathModule = require('../../src/utils/path.js');
+    originalGetConfigFilePath = pathModule.getConfigFilePath;
+    pathModule.getConfigFilePath = (filename: string) => {
+      if (filename === 'mcp_settings.json') {
+        return testSettingsPath;
+      }
+      return originalGetConfigFilePath(filename);
+    };
+
+    // Create test settings file
+    const testSettings = {
+      mcpServers: {
+        'test-server-1': {
+          command: 'echo',
+          args: ['test1'],
+        },
+        'test-server-2': {
+          command: 'echo',
+          args: ['test2'],
+        },
+      },
+      groups: [],
+      users: [{ username: 'admin', password: 'hash', isAdmin: true }],
+    };
+
+    // Ensure fixtures directory exists
+    const fixturesDir = path.dirname(testSettingsPath);
+    if (!fs.existsSync(fixturesDir)) {
+      fs.mkdirSync(fixturesDir, { recursive: true });
+    }
+
+    fs.writeFileSync(testSettingsPath, JSON.stringify(testSettings, null, 2));
+  });
+
+  afterAll(() => {
+    // Restore original function
+    if (originalGetConfigFilePath) {
+      // eslint-disable-next-line @typescript-eslint/no-var-requires
+      const pathModule = require('../../src/utils/path.js');
+      pathModule.getConfigFilePath = originalGetConfigFilePath;
+    }
+
+    // Clean up test file
+    if (fs.existsSync(testSettingsPath)) {
+      fs.unlinkSync(testSettingsPath);
+    }
+  });
+
+  beforeEach(() => {
+    // Clear the settings cache before each test
+    config.clearSettingsCache();
+
+    // Reset test settings file to clean state
+    const testSettings = {
+      mcpServers: {
+        'test-server-1': {
+          command: 'echo',
+          args: ['test1'],
+        },
+        'test-server-2': {
+          command: 'echo',
+          args: ['test2'],
+        },
+      },
+      groups: [],
+      users: [{ username: 'admin', password: 'hash', isAdmin: true }],
+    };
+
+    fs.writeFileSync(testSettingsPath, JSON.stringify(testSettings, null, 2));
+  });
+
+  it('should persist a newly created group to file', () => {
+    // Create a group
+    const groupName = 'integration-test-group';
+    const description = 'Test group for integration testing';
+    const servers = ['test-server-1'];
+
+    const newGroup = createGroup(groupName, description, servers);
+
+    expect(newGroup).not.toBeNull();
+    expect(newGroup?.name).toBe(groupName);
+
+    // Clear cache and reload settings from file
+    config.clearSettingsCache();
+
+    // Verify group was persisted to file
+    const savedSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    expect(savedSettings.groups).toHaveLength(1);
+    expect(savedSettings.groups[0].name).toBe(groupName);
+    expect(savedSettings.groups[0].description).toBe(description);
+    expect(savedSettings.groups[0].servers).toHaveLength(1);
+    expect(savedSettings.groups[0].servers[0]).toEqual({ name: 'test-server-1', tools: 'all' });
+  });
+
+  it('should persist multiple groups sequentially', () => {
+    // Create first group
+    const group1 = createGroup('group-1', 'First group', ['test-server-1']);
+    expect(group1).not.toBeNull();
+
+    // Clear cache
+    config.clearSettingsCache();
+
+    // Create second group
+    const group2 = createGroup('group-2', 'Second group', ['test-server-2']);
+    expect(group2).not.toBeNull();
+
+    // Clear cache and verify both groups are persisted
+    config.clearSettingsCache();
+    const savedSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    expect(savedSettings.groups).toHaveLength(2);
+    expect(savedSettings.groups[0].name).toBe('group-1');
+    expect(savedSettings.groups[1].name).toBe('group-2');
+  });
+
+  it('should preserve mcpServers when creating groups', () => {
+    // Get initial mcpServers
+    const initialSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    const initialServers = initialSettings.mcpServers;
+
+    // Create a group
+    const newGroup = createGroup('test-group', 'Test', ['test-server-1']);
+    expect(newGroup).not.toBeNull();
+
+    // Verify mcpServers are preserved
+    config.clearSettingsCache();
+    const savedSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    expect(savedSettings.mcpServers).toEqual(initialServers);
+    expect(savedSettings.groups).toHaveLength(1);
+  });
+
+  it('should allow deleting a persisted group', () => {
+    // Create a group
+    const newGroup = createGroup('temp-group', 'Temporary', ['test-server-1']);
+    expect(newGroup).not.toBeNull();
+
+    const groupId = newGroup!.id;
+
+    // Verify it's saved
+    config.clearSettingsCache();
+    let savedSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    expect(savedSettings.groups).toHaveLength(1);
+
+    // Delete the group
+    const deleted = deleteGroup(groupId);
+    expect(deleted).toBe(true);
+
+    // Verify it's deleted from file
+    config.clearSettingsCache();
+    savedSettings = JSON.parse(fs.readFileSync(testSettingsPath, 'utf8'));
+    expect(savedSettings.groups).toHaveLength(0);
+  });
+
+  it('should handle empty groups array correctly', () => {
+    // Get all groups when none exist
+    const groups = getAllGroups();
+    expect(groups).toEqual([]);
+
+    // Create a group
+    createGroup('first-group', 'First', ['test-server-1']);
+
+    // Clear cache and get groups again
+    config.clearSettingsCache();
+    const groupsAfterCreate = getAllGroups();
+    expect(groupsAfterCreate).toHaveLength(1);
+  });
+});

tests/services/dataServiceMerge.test.ts

@@ -0,0 +1,225 @@
+import { DataServiceImpl } from '../../src/services/dataService.js';
+import { DataServicex } from '../../src/services/dataServicex.js';
+import { McpSettings, IUser } from '../../src/types/index.js';
+
+describe('DataService mergeSettings', () => {
+  describe('DataServiceImpl', () => {
+    let service: DataServiceImpl;
+
+    beforeEach(() => {
+      service = new DataServiceImpl();
+    });
+
+    it('should merge all fields from newSettings into existing settings', () => {
+      const all: McpSettings = {
+        users: [
+          { username: 'admin', password: 'hash1', isAdmin: true },
+          { username: 'user1', password: 'hash2', isAdmin: false },
+        ],
+        mcpServers: {
+          'server1': { command: 'cmd1', args: [] },
+          'server2': { command: 'cmd2', args: [] },
+        },
+        groups: [
+          { id: '1', name: 'group1', servers: [], owner: 'admin' },
+        ],
+        systemConfig: {
+          routing: { enableGlobalRoute: true, enableGroupNameRoute: true },
+        },
+        userConfigs: {
+          user1: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
+        },
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          { id: '1', name: 'group1', servers: [], owner: 'admin' },
+          { id: '2', name: 'group2', servers: [], owner: 'admin' },
+        ],
+      };
+
+      const result = service.mergeSettings(all, newSettings);
+
+      // New groups should be present
+      expect(result.groups).toHaveLength(2);
+      expect(result.groups).toEqual(newSettings.groups);
+
+      // Other fields from 'all' should be preserved when not in newSettings
+      expect(result.users).toEqual(all.users);
+      expect(result.systemConfig).toEqual(all.systemConfig);
+      expect(result.userConfigs).toEqual(all.userConfigs);
+    });
+
+    it('should preserve fields not present in newSettings', () => {
+      const all: McpSettings = {
+        users: [{ username: 'admin', password: 'hash', isAdmin: true }],
+        mcpServers: {
+          'server1': { command: 'cmd1', args: [] },
+        },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+        systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          { id: '1', name: 'group1', servers: [], owner: 'admin' },
+          { id: '2', name: 'group2', servers: [], owner: 'admin' },
+        ],
+      };
+
+      const result = service.mergeSettings(all, newSettings);
+
+      // Groups from newSettings should be present
+      expect(result.groups).toEqual(newSettings.groups);
+
+      // Other fields should be preserved from 'all'
+      expect(result.users).toEqual(all.users);
+      expect(result.systemConfig).toEqual(all.systemConfig);
+    });
+
+    it('should handle undefined fields in newSettings', () => {
+      const all: McpSettings = {
+        users: [{ username: 'admin', password: 'hash', isAdmin: true }],
+        mcpServers: { 'server1': { command: 'cmd1', args: [] } },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        // groups is undefined
+      };
+
+      const result = service.mergeSettings(all, newSettings);
+
+      // Groups from 'all' should be preserved since newSettings.groups is undefined
+      expect(result.groups).toEqual(all.groups);
+      expect(result.users).toEqual(all.users);
+    });
+  });
+
+  describe('DataServicex', () => {
+    let service: DataServicex;
+
+    beforeEach(() => {
+      service = new DataServicex();
+    });
+
+    it('should merge all fields for admin users', () => {
+      const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
+
+      const all: McpSettings = {
+        users: [adminUser],
+        mcpServers: {
+          'server1': { command: 'cmd1', args: [] },
+        },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+        systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          { id: '1', name: 'group1', servers: [], owner: 'admin' },
+          { id: '2', name: 'group2', servers: [], owner: 'admin' },
+        ],
+        systemConfig: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
+      };
+
+      const result = service.mergeSettings(all, newSettings, adminUser);
+
+      // All fields from newSettings should be merged
+      expect(result.groups).toEqual(newSettings.groups);
+      expect(result.systemConfig).toEqual(newSettings.systemConfig);
+
+      // Users should be preserved from 'all' since not in newSettings
+      expect(result.users).toEqual(all.users);
+    });
+
+    it('should preserve groups for admin users when adding new groups', () => {
+      const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
+
+      const all: McpSettings = {
+        users: [adminUser],
+        mcpServers: { 'server1': { command: 'cmd1', args: [] } },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          { id: '1', name: 'group1', servers: [], owner: 'admin' },
+          { id: '2', name: 'group2', servers: [], owner: 'admin' },
+        ],
+      };
+
+      const result = service.mergeSettings(all, newSettings, adminUser);
+
+      // New groups should be present
+      expect(result.groups).toHaveLength(2);
+      expect(result.groups).toEqual(newSettings.groups);
+    });
+
+    it('should handle non-admin users correctly', () => {
+      const regularUser: IUser = { username: 'user1', password: 'hash', isAdmin: false };
+
+      const all: McpSettings = {
+        users: [regularUser],
+        mcpServers: { 'server1': { command: 'cmd1', args: [] } },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+        userConfigs: {},
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        systemConfig: {
+          routing: {
+            enableGlobalRoute: false,
+            enableGroupNameRoute: false,
+            enableBearerAuth: true,
+            bearerAuthKey: 'test-key',
+          },
+        },
+      };
+
+      const result = service.mergeSettings(all, newSettings, regularUser);
+
+      // For non-admin users, groups should not change
+      expect(result.groups).toEqual(all.groups);
+
+      // User config should be updated
+      expect(result.userConfigs).toBeDefined();
+      expect(result.userConfigs?.['user1']).toBeDefined();
+      expect(result.userConfigs?.['user1'].routing).toEqual(newSettings.systemConfig?.routing);
+    });
+
+    it('should preserve all fields from original when only updating systemConfig', () => {
+      const adminUser: IUser = { username: 'admin', password: 'hash', isAdmin: true };
+
+      const all: McpSettings = {
+        users: [adminUser],
+        mcpServers: { 'server1': { command: 'cmd1', args: [] } },
+        groups: [{ id: '1', name: 'group1', servers: [], owner: 'admin' }],
+        systemConfig: { routing: { enableGlobalRoute: true, enableGroupNameRoute: true } },
+      };
+
+      const newSettings: McpSettings = {
+        mcpServers: {},
+        systemConfig: { routing: { enableGlobalRoute: false, enableGroupNameRoute: false } },
+      };
+
+      const result = service.mergeSettings(all, newSettings, adminUser);
+
+      // Groups should be preserved from 'all' since not in newSettings
+      expect(result.groups).toEqual(all.groups);
+      // SystemConfig should be updated from newSettings
+      expect(result.systemConfig).toEqual(newSettings.systemConfig);
+      // Users should be preserved from 'all' since not in newSettings
+      expect(result.users).toEqual(all.users);
+      // mcpServers should be updated from newSettings (empty in this case)
+      // This is expected behavior - when mcpServers is explicitly provided, it replaces the old value
+      expect(result.mcpServers).toEqual(newSettings.mcpServers);
+    });
+  });
+});

tests/services/groupService.test.ts

@@ -0,0 +1,262 @@
+import { createGroup, getAllGroups, deleteGroup } from '../../src/services/groupService.js';
+import * as config from '../../src/config/index.js';
+import { McpSettings } from '../../src/types/index.js';
+
+// Mock the config module
+jest.mock('../../src/config/index.js', () => {
+  let mockSettings: McpSettings = {
+    mcpServers: {
+      'test-server': {
+        command: 'test',
+        args: [],
+      },
+    },
+    groups: [],
+    users: [],
+  };
+
+  return {
+    loadSettings: jest.fn(() => mockSettings),
+    saveSettings: jest.fn((settings: McpSettings) => {
+      mockSettings = settings;
+      return true;
+    }),
+    clearSettingsCache: jest.fn(),
+  };
+});
+
+// Mock the mcpService
+jest.mock('../../src/services/mcpService.js', () => ({
+  notifyToolChanged: jest.fn(),
+}));
+
+// Mock the dataService
+jest.mock('../../src/services/services.js', () => ({
+  getDataService: jest.fn(() => ({
+    filterData: (data: any[]) => data,
+    filterSettings: (settings: any) => settings,
+    mergeSettings: (all: any, newSettings: any) => newSettings,
+    getPermissions: () => ['*'],
+  })),
+}));
+
+describe('Group Service', () => {
+  beforeEach(() => {
+    // Reset mocks before each test
+    jest.clearAllMocks();
+
+    // Reset the mock settings to initial state
+    const mockSettings: McpSettings = {
+      mcpServers: {
+        'test-server': {
+          command: 'test',
+          args: [],
+        },
+        'test-server-2': {
+          command: 'test2',
+          args: [],
+        },
+      },
+      groups: [],
+      users: [],
+    };
+
+    (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+    (config.saveSettings as jest.Mock).mockImplementation((settings: McpSettings) => {
+      mockSettings.groups = settings.groups;
+      return true;
+    });
+  });
+
+  describe('createGroup', () => {
+    it('should create a new group and persist it', () => {
+      const groupName = 'test-group';
+      const description = 'Test group description';
+      const servers = ['test-server'];
+
+      const newGroup = createGroup(groupName, description, servers);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.name).toBe(groupName);
+      expect(newGroup?.description).toBe(description);
+      expect(newGroup?.servers).toHaveLength(1);
+      expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
+
+      // Verify saveSettings was called
+      expect(config.saveSettings).toHaveBeenCalled();
+
+      // Verify the settings passed to saveSettings include the new group
+      const savedSettings = (config.saveSettings as jest.Mock).mock.calls[0][0];
+      expect(savedSettings.groups).toHaveLength(1);
+      expect(savedSettings.groups[0].name).toBe(groupName);
+    });
+
+    it('should create a group with multiple servers', () => {
+      const groupName = 'multi-server-group';
+      const servers = ['test-server', 'test-server-2'];
+
+      const newGroup = createGroup(groupName, undefined, servers);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.servers).toHaveLength(2);
+      expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
+      expect(newGroup?.servers[1]).toEqual({ name: 'test-server-2', tools: 'all' });
+    });
+
+    it('should create a group with server configuration objects', () => {
+      const groupName = 'config-group';
+      const servers = [
+        { name: 'test-server', tools: 'all' },
+        { name: 'test-server-2', tools: ['tool1', 'tool2'] },
+      ];
+
+      const newGroup = createGroup(groupName, undefined, servers);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.servers).toHaveLength(2);
+      expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
+      expect(newGroup?.servers[1]).toEqual({ name: 'test-server-2', tools: ['tool1', 'tool2'] });
+    });
+
+    it('should filter out non-existent servers', () => {
+      const groupName = 'filtered-group';
+      const servers = ['test-server', 'non-existent-server'];
+
+      const newGroup = createGroup(groupName, undefined, servers);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.servers).toHaveLength(1);
+      expect(newGroup?.servers[0]).toEqual({ name: 'test-server', tools: 'all' });
+    });
+
+    it('should not create a group with duplicate name', () => {
+      const groupName = 'duplicate-group';
+
+      // Create first group
+      const firstGroup = createGroup(groupName, 'First group');
+      expect(firstGroup).not.toBeNull();
+
+      // Update the mock to include the first group
+      const mockSettings: McpSettings = {
+        mcpServers: {
+          'test-server': {
+            command: 'test',
+            args: [],
+          },
+        },
+        groups: [firstGroup!],
+        users: [],
+      };
+      (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+
+      // Try to create second group with same name
+      const secondGroup = createGroup(groupName, 'Second group');
+      expect(secondGroup).toBeNull();
+    });
+
+    it('should set owner to admin by default', () => {
+      const groupName = 'owned-group';
+
+      const newGroup = createGroup(groupName);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.owner).toBe('admin');
+    });
+
+    it('should set custom owner when provided', () => {
+      const groupName = 'custom-owned-group';
+      const owner = 'testuser';
+
+      const newGroup = createGroup(groupName, undefined, [], owner);
+
+      expect(newGroup).not.toBeNull();
+      expect(newGroup?.owner).toBe(owner);
+    });
+  });
+
+  describe('getAllGroups', () => {
+    it('should return all groups', () => {
+      const mockSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          {
+            id: '1',
+            name: 'group1',
+            servers: [],
+            owner: 'admin',
+          },
+          {
+            id: '2',
+            name: 'group2',
+            servers: [],
+            owner: 'admin',
+          },
+        ],
+        users: [],
+      };
+      (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+
+      const groups = getAllGroups();
+
+      expect(groups).toHaveLength(2);
+      expect(groups[0].name).toBe('group1');
+      expect(groups[1].name).toBe('group2');
+    });
+
+    it('should return empty array when no groups exist', () => {
+      const mockSettings: McpSettings = {
+        mcpServers: {},
+        users: [],
+      };
+      (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+
+      const groups = getAllGroups();
+
+      expect(groups).toEqual([]);
+    });
+  });
+
+  describe('deleteGroup', () => {
+    it('should delete a group by id', () => {
+      const mockSettings: McpSettings = {
+        mcpServers: {},
+        groups: [
+          {
+            id: 'group-to-delete',
+            name: 'Delete Me',
+            servers: [],
+            owner: 'admin',
+          },
+        ],
+        users: [],
+      };
+      (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+      (config.saveSettings as jest.Mock).mockImplementation((settings: McpSettings) => {
+        mockSettings.groups = settings.groups;
+        return true;
+      });
+
+      const result = deleteGroup('group-to-delete');
+
+      expect(result).toBe(true);
+      expect(config.saveSettings).toHaveBeenCalled();
+
+      // Verify the settings passed to saveSettings have the group removed
+      const savedSettings = (config.saveSettings as jest.Mock).mock.calls[0][0];
+      expect(savedSettings.groups).toHaveLength(0);
+    });
+
+    it('should return false when group does not exist', () => {
+      const mockSettings: McpSettings = {
+        mcpServers: {},
+        groups: [],
+        users: [],
+      };
+      (config.loadSettings as jest.Mock).mockReturnValue(mockSettings);
+
+      const result = deleteGroup('non-existent-id');
+
+      expect(result).toBe(false);
+    });
+  });
+});