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: fix incremental test and docs #3161

Merged
merged 10 commits into from
May 3, 2021

Conversation

keu
Copy link
Contributor

@keu keu commented Apr 30, 2021

What

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

How

Describe the solution

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

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

@keu keu requested review from sherifnada and removed request for michel-tricot and ChristopheDuong April 30, 2021 20:26
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.

LGTM % some comments


from setuptools import find_packages, setup
TEST_REQUIREMENTS = [
"pytest==6.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it be better to do something like ^6.1 rather than pin to a specific version?

@@ -0,0 +1,29 @@
# See [Source Acceptance Tests](https://docs.airbyte.io/contributing-to-airbyte/building-new-connector/source-acceptance-tests.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed for this PR but the number of common files between these templates is growing. Maybe we should have a common directory connector-templates/common which contains all common files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we will migrate to cookiecuttter we can have a single template for all types of connectors, solving this problem in such case

Copy link
Contributor

Choose a reason for hiding this comment

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

def parse_value(value, type_):
if type_ in ("datetime", "date-time"):
def parse_value(value: Any, type_: Set[str]):
if type_ & {"datetime", "date-time"}:
Copy link
Contributor

Choose a reason for hiding this comment

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

python is bonkers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can be expressed like this:

if type_.intersect({"datetime", "date-time"}):

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I slightly prefer that just because it's less cognitive load (intersect tells you exactly what it is doing)

try:
return self.get_property(path)["type"]
field = self.get_property(path)
type_ = field.get("format", field["type"])
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this format? very confusing to call it type


def get_cursor_value(self, record, cursor_path):
type_ = self.get_type_for_key_path(path=cursor_path)
value = reduce(lambda data, key: data[key], cursor_path, record)
return self.parse_value(value, type_)

@staticmethod
def parse_value(value, type_):
if type_ in ("datetime", "date-time"):
def parse_value(value: Any, type_: Set[str]):
Copy link
Contributor

Choose a reason for hiding this comment

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

are these parameters well named? it seems like value is actually JSON Schema type and type_ is format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type is something in between I would say, but ok, lets make format
value is an actual value, it can be 1, '1', 'sometext', 20.0, '2010-10-11'
format is what tells us how to parse it

keu and others added 3 commits May 3, 2021 12:43
…i/integration_tests/acceptance.py

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
…tance_test/utils/json_schema_helper.py

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
@keu keu merged commit 41ad3a9 into master May 3, 2021
@keu keu deleted the keu/acceptance-tests-fix-incremental-and-docs branch May 3, 2021 17:43
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.

None yet

3 participants