Test deletion and ALLOWED_GROUPS in integration tests

[aponcedeleonch]

• Add fetch MCP server installation alongside time server in integration tests
• Test server deletion by removing fetch server mid-workflow and verifying removal
• Run integration tests twice: once with fetch server present, once after deletion
• Add EXPECT_FETCH_ABSENT environment variable to control expected tools in tests
• Set WORKLOAD_POLLING_INTERVAL to 2 seconds for faster polling during tests
• Add verification step with sleep and status checks after deletion
• Add ALLOWED_GROUPS test to verify only serves in allowed groups are returned by mcp-optimizer

[claude]

PR Review: Integration Test Improvements

Summary

This PR enhances the integration test suite by adding server deletion testing and ALLOWED_GROUPS verification. The changes test dynamic server lifecycle management and group-based filtering.

✅ Strengths

• Good test coverage expansion: Tests both positive (fetch present) and negative (fetch absent) scenarios
• Realistic workflow testing: Server deletion mid-workflow mirrors real-world usage patterns
• Clear environment-based control: Using EXPECT_FETCH_ABSENT makes test behavior explicit
• Faster test execution: WORKLOAD_POLLING_INTERVAL=2 improves CI performance
• Group filtering validation: Verifies that ALLOWED_GROUPS=default correctly excludes test-group servers

🐛 Critical Bug

Line 171 in tests/integration_test.py: Typo "appedn" should be "append"

This will cause test failures when ENABLE_DYNAMIC_INSTALL is set.

⚠️ Issues & Concerns

1. Hardcoded sleep timing (.github/workflows/integration-tests.yml:122):

   • Uses "sleep 8" which is brittle and may fail under load or slow CI runners
   • Consider polling with timeout instead, or at minimum add a comment explaining why 8 seconds

2. Inconsistent data structure usage (tests/integration_test.py:169-171):

   • expected_tools is defined as a set on line 163
   • But lines 170-171 try to use .append() which doesn't exist for sets
   • Should use .add() to match the data structure

3. Container logs for deleted server (.github/workflows/integration-tests.yml:142-143):

   • Attempting to fetch logs from a deleted container may produce confusing output
   • Consider capturing logs before deletion or clarifying this is expected to fail

4. Test title typo: PR title has "Teste" instead of "Test"

🔒 Security

No security concerns identified. Environment variables are appropriately scoped.

⚡ Performance

The WORKLOAD_POLLING_INTERVAL=2 change is good for CI speed without compromising test reliability.

📝 Suggestions

1. Fix the critical bug on line 171 - change "appedn" to "add"
2. Fix data structure consistency: Use .add() instead of .append() for sets (line 170)
3. Add polling logic instead of sleep 8 for more reliable deletion verification
4. Move fetch container log capture to before deletion step
5. Add assertion to verify playwright tools are NOT present in the results (currently only implicitly tested)

Test Coverage

✅ Server installation
✅ Server deletion
✅ Group filtering (ALLOWED_GROUPS)
✅ Dynamic test expectations
⚠️ Missing explicit verification that excluded groups don't appear

Verdict

Needs changes - The typo on line 171 will cause test failures. Once fixed, this is a valuable addition to the test suite.

[aponcedeleonch]

@claude check again the newest commit. Verify if the fixes were applied. Make your comment short and precise

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Verification of Review Fixes

• Read latest version of changed files
• Verify critical bug fixes
• Provide concise summary

✅ All Critical Issues Fixed

Previous bugs resolved:

1. ✅ Line 171 typo fixed: Changed from "appedn" to "add" (tests/integration_test.py:170)
2. ✅ Data structure consistency fixed: Now correctly uses expected_tools.add() for set operations (line 170-171)

Code is now correct:

• Line 163: expected_tools is a set
• Lines 170-171: Properly uses .add() method for sets when ENABLE_DYNAMIC_INSTALL is set

Other observations:

• Sleep timing (8 seconds) remains hardcoded but acceptable for this use case
• Fetch container logs captured before deletion (line 118) - good practice
• ALLOWED_GROUPS test properly configured (line 64, 52-53)

Verdict

✅ Ready to merge - All critical bugs identified in the previous review have been fixed.

---

Job Run | Branch: integration-improvements

[therealnb]

Looks good