feat(python-sources): add unit integration testing utilities for simplification #43338

strosek · 2024-08-06T19:04:39Z

What

This PR tackles a few challenges we have when writing integration tests:

Too much duplicated code
Difficult to match request urls
Brittle assertions
Test data is too disorganized

Here is provided a new, more generalized way of creating integration tests that reduces size of tests by up to 75%, enhances thoroughness, makes mocking simpler and powerful, and makes matching requests much easier. The suite of woocommerce tests was migrated to show this in action.

How

Configure tests using functions and data, not classes
Use a single test to test all streams with similar structure, goal is one test per family of test cross-streams
Make test data easy to spot, don't hide behind a lot of functions/classes
Use standard mocker, which is more flexible
Don't repeat common code, place it in a common location
Provide more thorough and flexible, common assertions

Review guide

test_read.py - implements 22 test cases covering basic reads and a multiple reads test.
conftest.py - implements specific test configurations.
common.py - implements common helpers for a source.
utils.py - implements reusable test utilities.
assertion.py - implements reusable, thorough assertion functions.
data.py - implements utility function to load test JSON data files.

User Impact

None, just easier testing improves probability of quality.

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2024-08-06T19:04:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 14, 2024 0:51am

girarda · 2024-08-06T19:16:35Z

airbyte-cdk/python/airbyte_cdk/test/utils/reading.py

+from airbyte_cdk.test.entrypoint_wrapper import EntrypointOutput, read
+
+
+def catalog(stream_name: str, sync_mode: SyncMode) -> ConfiguredAirbyteCatalog:


It makes a ton of sense to me to bring these two methods in the CDK since they're reimplemented by all connectors!

I am personally skeptic about this one because of the "brittleness" I mentioned here. I feel like the builder is as readable as this method and I don't know what is the benefit of this method

I'd be fine with a builder. what I like about the current approach is we get to remove the duplicated _read static methods which is maybe not related to this specific function, so my bad

These functions are what many tests use, I just moved them. These are provided in a common location for people to use them when they don't need the full builder or an __init__() with default arguments. It's difficult to generalize everything, but these little functions come handy in many cases, if they are not handy, people can fall back to the builder.

airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/test_basic_read.py

pnilan

Looks clean. Nice work.

Do you think you'll add State assertions, too?

airbyte-cdk/python/airbyte_cdk/test/utils/assertions.py

airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/utils.py

# Conflicts: # airbyte-integrations/connectors/source-woocommerce/poetry.lock # airbyte-integrations/connectors/source-woocommerce/pyproject.toml # airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/test_coupons.py

maxi297

Left a couple of comments. What I like most about this PR is pushing more validation logic in the CDK

airbyte-cdk/python/airbyte_cdk/test/utils/assertions.py

airbyte-cdk/python/airbyte_cdk/test/utils/data.py

maxi297 · 2024-08-09T13:14:50Z

airbyte-cdk/python/airbyte_cdk/test/utils/reading.py

+    return CatalogBuilder().with_stream(stream_name, sync_mode).build()
+
+
+def read_records(


I find this interface brittle. If at some point we need more than just the stream_name, sync_mode to describe the catalog, we will have to update this interface. Why not pass the catalog directly? We have a builder that should ease the process of instantiating it

Same as the other comment above in reading.py

maxi297 · 2024-08-09T13:21:55Z

airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/test_read.py

+    "stream_name, num_records, http_calls",
+    [("orders", 2, orders_empty_last_page())]
+)
+def test_read_with_multiple_pages_with_empty_last_page_successfully(stream_name, num_records, http_calls) -> None:


What ensures that we trigger pagination here?

The freeze date that goes beyond the first page of one month, and creates second http call. Do you think that is not reliable?

One of the benefit from the HttpMocker was that it validated that all the HTTP requests that were mocked were called (at least when it is used as an annotation but we have also started to use it through the unittest.TestCase so we should probably add the same logic to the exit

That's a good point. However, the intention of this modeling of tests is not to provide all the features but provide a simpler/flexible alternative. For now I added a call-count assertion. Is it OK if we add such features in subsequent improvements?

Why not use the HttpMocker if it's already implemented there? Which makes me think that I don't see from airbyte_cdk.test.utils.http_mocking import register_mock_responses available on this branch

Oh, sorry about the missing file. I'm not using that mocker because it requires the specific Request type objects and I wanted to make it simpler. The vanilla http_mocking.Mocker can use simple requests and regexes that make it easier to match things.

maxi297

Well... the consequence of using private interfaces!

maxi297 · 2024-08-12T17:51:21Z

airbyte-integrations/connectors/source-amazon-seller-partner/unit_tests/integration/utils.py

@@ -11,7 +11,7 @@
 from airbyte_cdk.test.catalog_builder import CatalogBuilder
 from airbyte_cdk.test.entrypoint_wrapper import EntrypointOutput, read
 from airbyte_cdk.test.mock_http import HttpMocker
-from airbyte_cdk.test.mock_http.response_builder import _get_unit_test_folder
+from airbyte_cdk.test.mock_http.response_builder import get_unit_test_folder


Oops, this is unfortunate! We will need to update the CDK version before doing that. It uses CDK version 0.90.0 which means a lot of breaking changes.

The options I see are:

Pay the debt of importing private methods now by updating the CDK (this seems to increase the scope way too much)

Keep _get_unit_test_folder and have it just call get_unit_test_folder internally with a big red comment in the CDK and source-amazon-seller-partner that says why we have this and when we can clean this. In that case, if you want to move get_unit_test_folder to data, that works too

I prefer the second option personally but will let you choose.

OK, going with the second option.

maxi297

LGTM! Thanks for tackling the comments diligently

octavia-approvington · 2024-08-14T17:15:49Z

Lets merge it

Draft basic read integration test

562be14

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/woocommerce labels Aug 6, 2024

girarda reviewed Aug 6, 2024

View reviewed changes

pnilan reviewed Aug 6, 2024

View reviewed changes

airbyte-cdk/python/airbyte_cdk/test/utils/assertions.py Outdated Show resolved Hide resolved

airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/utils.py Outdated Show resolved Hide resolved

Erick Corona added 4 commits August 6, 2024 18:47

Add tests for WooCommerce simple streams

927f0f6

Generalize test structure, test all streams

a298a11

Complete test suite, remove unused code

39714a6

Merge branch 'master' into strosek/test_utils

6d16578

# Conflicts: # airbyte-integrations/connectors/source-woocommerce/poetry.lock # airbyte-integrations/connectors/source-woocommerce/pyproject.toml # airbyte-integrations/connectors/source-woocommerce/unit_tests/integration/test_coupons.py

vercel bot deployed to Preview August 8, 2024 14:00 View deployment

Fix matcher in multiple pages test

5cc3e6a

strosek self-assigned this Aug 8, 2024

strosek changed the title ~~Draft basic read integration test~~ feat(python-sources): make woocommerce integration tests simple, add test utilities Aug 8, 2024

strosek changed the title ~~feat(python-sources): make woocommerce integration tests simple, add test utilities~~ feat(python-sources): make integration tests simple, add test utilities, migrate woocommerce tests Aug 9, 2024

Erick Corona and others added 3 commits August 8, 2024 20:07

Reduce code duplication, apply format code

d5a79b1

Add docstrings to test utils

f30ddcb

Merge branch 'master' into strosek/test_utils

f9dd62f

vercel bot deployed to Preview August 9, 2024 02:55 View deployment

strosek marked this pull request as ready for review August 9, 2024 03:04

strosek requested review from maxi297 and girarda August 9, 2024 03:04

maxi297 reviewed Aug 9, 2024

View reviewed changes

Make registered mock method configurable, address comments

c9bb9ae

strosek requested a review from maxi297 August 9, 2024 17:49

Erick Corona and others added 4 commits August 9, 2024 12:26

Add missing http_mocking module

f04513a

Add missing http_mocking module

476f75b

Apply formatting

b1ecee5

Merge branch 'master' into strosek/test_utils

840871b

restore unit tests to split PR in two

d485355

octavia-squidington-iii removed the area/connectors Connector related issues label Aug 12, 2024

strosek changed the title ~~feat(python-sources): make integration tests simple, add test utilities, migrate woocommerce tests~~ feat(python-sources): add unit integration testing utilities for simplification Aug 12, 2024

Erick Corona added 2 commits August 12, 2024 11:24

Update JSON reference in read_resource_file_contents

330bf1d

Refactor get_unit_tests_folder to be public and reused

00fc44e

octavia-squidington-iii added area/connectors Connector related issues connectors/source/amazon-seller-partner labels Aug 12, 2024

Erick Corona and others added 2 commits August 12, 2024 11:51

Fix file path type annotation

a158d09

Merge branch 'master' into strosek/test_utils

58d40b1

maxi297 reviewed Aug 12, 2024

View reviewed changes

vercel bot deployed to Preview August 12, 2024 18:01 View deployment

Erick Corona and others added 2 commits August 12, 2024 14:35

Revert back to using private get_unit_test_folder

ea2dda0

Merge branch 'master' into strosek/test_utils

315d2ea

vercel bot deployed to Preview August 12, 2024 20:42 View deployment

strosek and others added 2 commits August 12, 2024 14:53

Merge branch 'master' into strosek/test_utils

2efec5d

Fix formatting

3888783

strosek requested a review from maxi297 August 12, 2024 22:05

Merge branch 'master' into strosek/test_utils

2747636

vercel bot deployed to Preview August 13, 2024 13:36 View deployment

Move get_unit_test_folder to utils/data

8fd4435

maxi297 approved these changes Aug 13, 2024

View reviewed changes

Merge branch 'master' into strosek/test_utils

c779bf0

vercel bot deployed to Preview August 13, 2024 17:40 View deployment

Merge branch 'master' into strosek/test_utils

82ea347

vercel bot deployed to Preview August 14, 2024 12:51 View deployment

octavia-approvington approved these changes Aug 14, 2024

View reviewed changes

octavia-approvington merged commit 198f83e into master Aug 14, 2024
33 of 40 checks passed

octavia-approvington deleted the strosek/test_utils branch August 14, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(python-sources): add unit integration testing utilities for simplification #43338

feat(python-sources): add unit integration testing utilities for simplification #43338

strosek commented Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

girarda Aug 6, 2024

maxi297 Aug 9, 2024

girarda Aug 9, 2024 •

edited

Loading

strosek Aug 9, 2024 •

edited

Loading

pnilan left a comment

maxi297 left a comment

maxi297 Aug 9, 2024

strosek Aug 9, 2024

maxi297 Aug 9, 2024

strosek Aug 9, 2024

maxi297 Aug 9, 2024 •

edited

Loading

strosek Aug 9, 2024 •

edited

Loading

maxi297 Aug 9, 2024

strosek Aug 9, 2024

maxi297 left a comment

maxi297 Aug 12, 2024

strosek Aug 12, 2024

maxi297 left a comment

octavia-approvington commented Aug 14, 2024

		from airbyte_cdk.test.entrypoint_wrapper import EntrypointOutput, read


		def catalog(stream_name: str, sync_mode: SyncMode) -> ConfiguredAirbyteCatalog:

		return CatalogBuilder().with_stream(stream_name, sync_mode).build()


		def read_records(

feat(python-sources): add unit integration testing utilities for simplification #43338

feat(python-sources): add unit integration testing utilities for simplification #43338

Conversation

strosek commented Aug 6, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

strosek Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

pnilan left a comment

Choose a reason for hiding this comment

maxi297 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

maxi297 Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

strosek Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

octavia-approvington commented Aug 14, 2024

strosek commented Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

girarda Aug 9, 2024 •

edited

Loading

strosek Aug 9, 2024 •

edited

Loading

maxi297 Aug 9, 2024 •

edited

Loading

strosek Aug 9, 2024 •

edited

Loading