[refactor] chore: support ci mutli tests #292
Conversation
|
Hi, @kitalkuyo-gita. Thanks for your contribution. In the current implementation, only the integration tests for Ollama have been isolated, which limits the scalability of the integration tests. We would prefer to isolate the tests for the entire integration module, rather than only for Ollama. |
wenjin272
left a comment
wenjin272
left a comment
There was a problem hiding this comment.
Hi, @kitalkuyo-gita. Thanks for your work, LGTM, only one minor comment.
And there is conflict with main branch, you may need apply rebase on it.
it's ready |
wenjin272
left a comment
wenjin272
left a comment
There was a problem hiding this comment.
Hi, @kitalkuyo-gita. I'm not sure whether some code was missed in the last review or there have been recent changes, so I've left some new comments.
In addition, in git practices, there are discussions regarding the use of merge versus rebase. However, in Flink-Agents, we use rebase to ensure the linearity of the main branch. So you need reorganize the commits of this pr and rebase on the main branch, not merge it.
| env: | ||
| PYTEST_SKIP_MARKERS: "integration" | ||
|
|
||
| ollama_integration_tests: |
There was a problem hiding this comment.
Maybe this can be removed.
| from flink_agents.plan.tools.function_tool import FunctionTool, from_callable | ||
|
|
||
| # Mark all tests in this module as ollama tests | ||
| pytestmark = pytest.mark.ollama |
There was a problem hiding this comment.
This should be pytest.mark.ollama.
| def test_ollama_embedding_setup() -> None: | ||
| """Test embedding functionality with OllamaEmbeddingModelSetup.""" | ||
| # Use longer timeout for embedding in CI environment (slower resources) | ||
| request_timeout = 120.0 if os.environ.get("CI") else 30.0 |
There was a problem hiding this comment.
This request timeout could also happen in local environment, so can uniform to 120.0
| def test_openai_embedding_model() -> None: # noqa: D103 | ||
| connection = OpenAIEmbeddingModelConnection( | ||
| name="openai", api_key=api_key | ||
| name="openai", api_key=api_key or "fake-key" |
There was a problem hiding this comment.
If this test is not skipped, the api_key must not be None, the or "fake-key" maybe useless.
| api_key=api_key, | ||
| tenant=tenant, | ||
| database=database, | ||
| api_key=api_key or "fake-key", |
|
|
||
|
|
||
| @pytest.mark.skipif(api_key is None, reason="TEST_API_KEY is not set") | ||
| @pytest.mark.integration |
There was a problem hiding this comment.
Have marked all tests in this module above, this decorator maybe not needed.
GreatEugenius
added
priority/major
3 participants
Purpose of change
Related to issues-161
Tests
API
Documentation