feat: add integration test case for flow execution#315
feat: add integration test case for flow execution#315weijinglin wants to merge 4 commits intoapache:mainfrom
Conversation
coderzc
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I reviewed this PR and found two issues that should be addressed before merge:
- [P1] New integration test is not executed in CI
- File: .github/workflows/hugegraph-llm.yml (Run integration tests step)
- test_flows_integration.py is added in this PR, but the workflow still runs only three explicit integration files. So the new tests are not part of PR checks and provide no regression protection.
- Suggestion: include the new file in the pytest command, or switch to directory-level collection with markers.
- [P2] Missing external-service skip guard in new integration tests
- File: hugegraph-llm/src/tests/integration/test_flows_integration.py
- The tests call real flows directly (BUILD_VECTOR_INDEX / GRAPH_EXTRACT / RAG_* / TEXT2GREMLIN) without should_skip_external()-style gating or mocks. Once enabled in CI/local integration runs, they can become flaky and environment-dependent.
- Suggestion: add explicit skip conditions for external dependencies, or stabilize with mocks/stubs for non-deterministic external calls.
Please address these and I can re-review quickly.
Thank u for your suggestions. For P1, I plan to add a end-to-end test for all the test cases in the app demo, which need api-key to launch the basic test. So I don't add the test scripts to the CI. I wanna add a contributed.md for this project and call for all the contributor to check the basic functionality of this project. What about your opinion? For P2, I don't simulate the external api because of the complexity of the workflow (caused by the nondeterminism of llm), so this test scripts is not added to the CI procedure. I'm sorry I didn't reply to you in a timely manner. |
No description provided.