Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standard Tests: create modular standard test suite v2 scaffolding #2702

Merged
merged 9 commits into from
Apr 20, 2021

Conversation

keu
Copy link
Contributor

@keu keu commented Apr 1, 2021

closes #2180

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.

How this actually works

I tried to keep things simple and at the same time aligned with pytest way of doing things. The latter allow us to override and/or extend behaviour by customizing fixtures in the code if needed, thuse our tests will have less surprises for people familiar with pytest.
To execute test I had two controversial approaches:

  • run from standard test package and point to configuration file with connector/test settings
  • run from connector's package, i.e. create a file with test and import standard tests from test package

I choice the first approach because it requires less files.

The bread and butter of this framework is fixture. Each test suite has set of inputs, multiple sets will result in parameterized test suite. The fixture called inputs represents such set of inputs, its value depends on which test suits we execute at the moment and the content of the config file.

typical config file:

connector_image: airbyte/source-hubspot:dev
tests:
  core:
    - config_path: "secrets/config.json"
      invalid_config_path: "secrets/invalid_config.json"
      spec_path: "source_hubspot/spec.json"
      configured_catalog_path: "sample_files/configured_catalog.json"
    - config_path: "secrets/config.json"
      invalid_config_path: "secrets/invalid_config.json"
      spec_path: "source_hubspot/spec.json"
      configured_catalog_path: "sample_files/configured_catalog.json"
  incremental: []
  full_refresh: []

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. test.java
  2. component.ts
  3. the rest

@keu keu added the zazmic label Apr 1, 2021
@keu keu self-assigned this Apr 1, 2021
@auto-assign auto-assign bot requested review from jrhizor and michel-tricot April 1, 2021 14:13
@@ -0,0 +1,6 @@
connector_image: airbyte/source-hubspot:0.1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we going to inject the version?

Copy link
Contributor Author

@keu keu Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest I don't know, this way of configuring standard test suite was in original requirements,
@sherifnada please give me a hand here :)

offtopic: it is relatively easy to override tag/version from custom command-option (something like pytest --connector="airbyte/source-hubspot:0.1.0")

SOFTWARE.
"""

import setuptools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we move to poetry? :)

Copy link
Contributor Author

@keu keu Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, poetry has some issues with dependencies specified as local relative paths (python-poetry/poetry#34)
we need to spend some time to investigate all pros and cons

from .utils import full_refresh_only_catalog, ConnectorRunner


class TestBaseInterface:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it StandardTestBaseInterface ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is a verb here.
nevertheless it was renamed to TestCore to align with configuration file

@keu keu requested a review from sherifnada April 2, 2021 19:07
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very exciting! 🎉 a lot of my comments can be addressed in follow up issues/separate PRs. There's a few comments that should be addressed before merging, but it's probably best to start separating new features/requests into different PRs.

@@ -0,0 +1,55 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a clearer name for this file is config_models.py? or put it in a models/configs.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, I don't see why models? this is configuration of standard_test package right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, it's the configuration's model. Model here just means the expected schema. This is not the actual config object. Another name is potentially spec but model is more common

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is an actual config object. The fact that we use pydantic to simplify it deserialization is nice feature. There is no need to have extra class to have a configuration. You can check another typical usage of Config object in pydantic docs. Maybe I'm missing something here (too pythonic way of thinking...dunno), but I being using pydantic models as configs in all my projects without any extra layers on top of it.

spec_path: str = spec_path


class ConnectionTestConfig(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused about this config.

It seems every time we need to create a connection test, we need to test for both success and failure.
Why don't we have

config_path: str
success: bool

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, success -> status, where status is enum of succeed, failed, exception

keu and others added 3 commits April 19, 2021 10:56
* fixes and address comments from @sherifnada

* fix tests

* fix leaking of secrets in the output

* fix package path

* fix imports

* hide secrets

* fix logs

* fix sequential read test

* fix build

* format

Co-authored-by: Eugene Kulak <kulak.eugene@gmail.com>
Co-authored-by: Eugene Kulak <kulak.eugene@gmail.com>
* improve debug output

* annotation

* improve output, add expected_records fixture

Co-authored-by: Eugene Kulak <kulak.eugene@gmail.com>
@keu keu changed the title Create modular standard test suite v2 scaffolding Standard Tests: create modular standard test suite v2 scaffolding Apr 20, 2021
@keu keu merged commit 1755847 into master Apr 20, 2021
@keu keu deleted the keu/standard-python-tests branch April 20, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create modular standard test suite v2 scaffolding
4 participants