Add test coverage for workflows router endpoints#374
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Clean PR — the 4 new tests are correct and follow the existing patterns in the file.
Minor observations (non-blocking):
-
Pre-existing docstring bug (not introduced by this PR):
test_workflow_disable_requires_auth(line 55) has docstringPOST /api/workflows/{id}/disablebut the actual route isDELETE /api/workflows/{id}and the test correctly usestest_client.delete. The newtest_workflow_disable_authenticatedgets this right. Consider fixing the old docstring in a follow-up. -
Coverage gap to be aware of: The 3 authenticated tests only exercise the 404 "workflow not in catalog" path. The happy paths for enable/disable/executions all require a live n8n connection, so testing them would need
aiohttpmocking (themock_aiohttp_sessionfixture in conftest.py exists for this). Not a blocker for this PR since it's explicitly filling the auth + 404 gaps, but worth noting for future work.
No CLAUDE.md violations, no bugs, no security issues. LGTM.
Summary
Motivation
Workflows router had 4 endpoints but incomplete test coverage:
DELETE /api/workflows/{id}- zero testsGET /api/workflows/{id}/executions- zero testsPOST /api/workflows/{id}/enable- only auth test, missing authenticated testAll other routers have comprehensive test coverage following the pattern: auth enforcement tests + authenticated endpoint tests.
Changes
Added 4 new tests:
test_workflow_executions_requires_auth- auth enforcement for executions endpointtest_workflow_enable_authenticated- authenticated test (404 on missing workflow)test_workflow_disable_authenticated- authenticated test for DELETE endpointtest_workflow_executions_authenticated- authenticated test for executions endpointTesting
Tests follow the same pattern as other routers: