chore: refactor and add components integration tests #3607

nicoloboschi · 2024-08-28T22:15:57Z

This PR moves and add some tests.
The final goal is to have the following features in CI:

Run both unit and integration tests that don't require external credentials on every PR
Run both unit and integration tests that required external credentials with the scheduled job

In order to achieve this, we use the api_key_required pytest markers to mark tests that require an external api key (e.g. openai api key)

In this PR I've added 3 different baselines for new kind of tests, all under integration:

components tests: test a single component, asserting the output with some given inputs. This will run as single-component flow, enabling the whole engine
flows tests: test a flow built from current version of the components, in a programmatic way (thanks to the recent work of @ogabrielluiz ). This also verifies that with some inputs the outputs is always the same for a given flow.
backward tests: download starter projects (and others in the future) from older versions of langflow and run them with the current engine. This will help us in catching incompatible changes

The expectation is that now on every PR modifying a component input or output must cover the change with one or more of these tests.

It's important to note that some integration tests do not require any api keys at all and they are run as part of PR validation.

github-actions · 2024-08-28T22:16:51Z

Pull Request Validation Report

This comment is automatically generated by Conventional PR

Whitelist Report

Whitelist	Active	Result
Pull request is a draft and should be ignored	✅	✅
Pull request is made by a whitelisted user and should be ignored	❌	❌
Pull request is submitted by a bot and should be ignored	✅	❌
Pull request is submitted by administrators and should be ignored	❌	❌

Result

Pull request matches with one (or more) enabled whitelist criteria. Pull request validation is skipped.

_{Last Modified at 28 Aug 24 22:16 UTC}

aws-amplify-sa-east-1 · 2024-08-28T22:19:21Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3607.dmtpw4p5recq1.amplifyapp.com

.github/workflows/python_test.yml

jordanrfrazier · 2024-08-28T23:20:19Z

Makefile

@@ -148,9 +148,20 @@ else
 		$(args)
 endif

+unit_tests_api_keys: ## run unit tests only with api key tests


I don't think we should classify any tests that require API keys as unit tests.

Then for integration tests, we also try the following naming pattern. Opinions?

integration_tests: ## Run all integration tests integration_tests_api_key_required_only: ## Run only integration tests that require api keys integration_tests_no_api_key_required_only: ## ..

jordanrfrazier · 2024-08-28T23:21:55Z

src/backend/base/langflow/graph/graph/base.py

-            for _component in component._components:
-                self.add_component(_component._id, _component)
+        self.vertex_map[component_id] = vertex
+        return component_id



I don't know the graph code well enough, but this seems like a big change. Can you explain what the bug was here and what this change fixes?

jordanrfrazier · 2024-08-28T23:22:27Z

src/backend/base/langflow/graph/graph/base.py

                        for predecessor in self.predecessor_map[neighbor]:
                            if predecessor not in queue and predecessor not in visited:
                                queue.append(predecessor)

            current_layer += 1  # Next layer
+        logger.debug(f"before refine {str(layers)}")


prob remove some of these debugs or make them more descriptive if you want to keep

jordanrfrazier · 2024-08-28T23:22:57Z

src/backend/tests/api_keys.py

@@ -0,0 +1,34 @@
+import os.path
+
+# we need to import tmpdir


is this leftover?

jordanrfrazier · 2024-08-28T23:23:54Z

src/backend/tests/integration/backward_compatibility/test_starter_projects.py

+
+@pytest.mark.asyncio
+@pytest.mark.api_key_required
+async def test_1_0_15_basic_prompting():


very cool, I like this compromise of testing actual json and not storing a full folder of flows in our repo

jordanrfrazier · 2024-08-28T23:24:33Z

src/backend/tests/integration/components/astra/test_astra_component.py

    )
-    component.build_vector_store()
+    print(results)


remove? assert results not error?

jordanrfrazier · 2024-08-28T23:26:16Z

src/backend/tests/integration/components/astra/test_astra_component.py

-    not check_env_vars("ASTRA_DB_APPLICATION_TOKEN", "ASTRA_DB_API_ENDPOINT"),
-    reason="missing astra env vars",
-)
-@pytest.mark.parametrize("astra_fixture", [SEARCH_COLLECTION], indirect=True)


Curious if/how the tests work without these parameters

jordanrfrazier · 2024-08-28T23:26:43Z

src/backend/tests/unit/components/models/test_ChatOllama_component.py

assume you just haven't done these changes / will have someone do them as a follow up

src/backend/tests/integration/utils.py

ogabrielluiz · 2024-08-29T13:34:44Z

src/backend/base/langflow/graph/graph/base.py

-        if _id in self.vertex_map:
-            return
+    def add_component(self, component: "Component", component_id: Optional[str] = None) -> str:
+        component_id = component_id or str(component.name + "-" + str(uuid.uuid4()))


If the user does not set an Id, the Component creates one automatically.

langflow/src/backend/base/langflow/custom/custom_component/component.py

Line 64 in af05228

config |= {"_id": f"{self.__class__.__name__}-{nanoid.generate(size=5)}"}

If you still think this should happen here as well, try using the same logic with nanoid

aligned to the existing logic, thanks

ogabrielluiz · 2024-08-29T13:37:56Z

src/backend/tests/integration/backward_compatibility/test_starter_projects.py

+
+@pytest.mark.asyncio
+@pytest.mark.api_key_required
+async def test_1_0_15_basic_prompting():


ogabrielluiz

LGTM

jordanrfrazier · 2024-08-30T19:41:54Z

.github/workflows/python_test.yml

+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ inputs.branch || github.ref }}
+      - name: Setup Node.js


I don't believe integration tests need node

jordanrfrazier · 2024-08-30T19:42:28Z

src/backend/base/langflow/custom/custom_component/component.py

@@ -61,6 +61,7 @@ def __init__(self, **kwargs):
        self._output_logs = {}
        config = config or {}
        if "_id" not in config:
+            print("generating id" + self.__class__.__name__)


jordanrfrazier · 2024-08-30T19:42:51Z

src/backend/base/langflow/graph/graph/base.py

@@ -1430,6 +1434,8 @@ async def _execute_tasks(self, tasks: list[asyncio.Task], lock: asyncio.Lock) ->
            # This could usually happen with input vertices like ChatInput
            self.run_manager.remove_vertex_from_runnables(v.id)

+            logger.debug(f"Vertex {v.id}, result: {v._built_result}, object: {v._built_object}")


jordanrfrazier · 2024-08-30T19:46:47Z

src/backend/tests/integration/components/astra/test_astra_component.py

-    not check_env_vars("ASTRA_DB_APPLICATION_TOKEN", "ASTRA_DB_API_ENDPOINT"),
-    reason="missing env vars",
-)
+@pytest.mark.api_key_required


Is the split of api_key_required vs. no_api_key_required enough? (I think for now, yes). In the future, it would be nice if CI automatically ran specific integration tests if it detected changes to specific components (for people with access to API Keys, i.e. us).

jordanrfrazier · 2024-08-30T19:48:17Z

src/backend/tests/integration/components/astra/test_astra_component.py

+                clazz=TextToData, inputs={"text_data": ["test1", "test2"]}, output_name="data"
+            ),
+            "embedding": ComponentInputHandle(
+                clazz=OpenAIEmbeddingsComponent,


I would err on using MockEmbeddings here, just as a best practice pattern. No need to introduce new dependencies or integrations if not necessary or explicitly testing them.

jordanrfrazier · 2024-08-30T19:49:25Z

src/backend/tests/unit/components/models/test_ChatOllama_component.py

Is this intentional - do you have plans to restore this in a follow up?

jordanrfrazier · 2024-08-30T19:50:21Z

src/backend/tests/unit/test_endpoints.py

@@ -427,7 +427,6 @@ def test_build_vertex_invalid_vertex_id(client, added_flow_with_prompt_and_histo
    assert response.status_code == 500


-@pytest.mark.api_key_required
 def test_successful_run_no_payload(client, simple_api_test, created_api_key):
    headers = {"x-api-key": created_api_key.api_key}


Does this test require API keys? Maybe this was the one you were referring to as unit test that required an API Key, which I understand is a bit ambiguous now.

no, it requires a langflow api key, not a credential api key

jordanrfrazier

Please take a look at previous comments and see what you'd like to update before merging, thanks

* improve inegration tests * add fixes * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

github-actions bot added the ignore-for-release label Aug 28, 2024

nicoloboschi force-pushed the int-tests branch from f257dd3 to 74adf65 Compare August 28, 2024 22:17

nicoloboschi force-pushed the int-tests branch 2 times, most recently from 9661365 to b16bd7f Compare August 28, 2024 22:52

jordanrfrazier reviewed Aug 28, 2024

View reviewed changes

ogabrielluiz requested changes Aug 29, 2024

View reviewed changes

nicoloboschi force-pushed the int-tests branch from f978c05 to 2866467 Compare August 29, 2024 22:48

nicoloboschi marked this pull request as ready for review August 29, 2024 22:48

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Aug 29, 2024

nicoloboschi requested review from ogabrielluiz and jordanrfrazier August 29, 2024 22:48

ogabrielluiz approved these changes Aug 30, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 30, 2024

jordanrfrazier reviewed Aug 30, 2024

View reviewed changes

jordanrfrazier approved these changes Aug 30, 2024

View reviewed changes

nicoloboschi force-pushed the int-tests branch from fc9f0d7 to ee5e5a9 Compare September 2, 2024 11:45

improve inegration tests

7483592

nicoloboschi force-pushed the int-tests branch from 912e683 to 7483592 Compare September 2, 2024 11:54

add fixes

97b523a

nicoloboschi force-pushed the int-tests branch from 1ad4312 to 97b523a Compare September 2, 2024 13:06

[autofix.ci] apply automated fixes

eab7f06

nicoloboschi merged commit 96872f3 into langflow-ai:main Sep 2, 2024
28 of 29 checks passed

joaoguilhermeS mentioned this pull request Sep 25, 2024

Conflict with pymongo and bson packages in MongoDB VectorStore Component #3912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor and add components integration tests #3607

chore: refactor and add components integration tests #3607

nicoloboschi commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

aws-amplify-sa-east-1 bot commented Aug 28, 2024

jordanrfrazier Aug 28, 2024 •

edited

Loading

jordanrfrazier Aug 28, 2024

jordanrfrazier Aug 28, 2024

jordanrfrazier Aug 28, 2024

jordanrfrazier Aug 28, 2024

ogabrielluiz Aug 29, 2024

jordanrfrazier Aug 28, 2024

jordanrfrazier Aug 28, 2024

jordanrfrazier Aug 28, 2024

ogabrielluiz Aug 29, 2024

nicoloboschi Aug 29, 2024

ogabrielluiz Aug 29, 2024

ogabrielluiz left a comment

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

jordanrfrazier Aug 30, 2024

nicoloboschi Sep 2, 2024

jordanrfrazier left a comment

chore: refactor and add components integration tests #3607

chore: refactor and add components integration tests #3607

Conversation

nicoloboschi commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

Pull Request Validation Report

Whitelist Report

aws-amplify-sa-east-1 bot commented Aug 28, 2024

jordanrfrazier Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogabrielluiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanrfrazier left a comment

Choose a reason for hiding this comment

jordanrfrazier Aug 28, 2024 •

edited

Loading