🐛 source-file: fix integration tests #31152
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
pedroslopez
changed the title
| @@ -57,7 +57,7 @@ def with_python_base(context: PipelineContext, python_version: str = "3.10") -> | |||
| sh_dash_c( | |||
| [ | |||
| "apt-get update", | |||
| "apt-get install -y build-essential cmake g++ libffi-dev libstdc++6 git", | |||
| "apt-get install -y build-essential cmake g++ libffi-dev libstdc++6 git curl", | |||
There was a problem hiding this comment.
required to be able to install docker
| @@ -82,6 +82,7 @@ def with_testing_dependencies(context: PipelineContext) -> Container: | |||
|
|
|||
| return ( | |||
| python_environment.with_exec(["pip", "install"] + CONNECTOR_TESTING_REQUIREMENTS) | |||
| .with_exec(sh_dash_c(["curl -fsSL https://get.docker.com | sh"])) | |||
There was a problem hiding this comment.
python test containers now include docker
There was a problem hiding this comment.
So the use case in here is we want to be able to use docker in tests to spin up containers nessessary for certain test environments. e.g. source-file needs docker compose for its test "remote" file server
(@pedroslopez do I have that right?)
In any sense looking at the code we already have I see with_global_dockerd_service and with_docker_cli inside the environments.py mega file.
But I dont think those are useful here for installing docker-compose.
(@alafanechere can you fact check me here?)
So if thats all true then I think were on the right track but should have a modification
- Lets not use curl
- Lets install docker / docker-compose via apt-get
There was a problem hiding this comment.
FYI: In #30474 this with_testing_dependencies function is removed. Connectors test will run inside the connector container on top of which we'd install its testing dependencies according to its setup.py.
I believe docker as a system dependency should not be added to this function, nor to a future "pre/post" hook for source-file.
If we need docker-compose for testing I suggest to install the pip distributed docker-compose by declaring pytest-docker[docker-compose-v1] as a test requirement. see https://github.com/avast/pytest-docker .
If we really want to use the latest docker-compose version I suggest to only install the docker-compose plugin. This can be added to with_testing_dependencies in this PR but eventually added to a pre/post build hook following #30474 .
I'd prefer that we stick to the pip distributed docker compose as it will not require a pre/post build hook implementation for this connector. And I believe it's fine to use an old docker compose version in this context.
| TEST_REQUIREMENTS = ["requests-mock~=1.9.3", "pytest~=6.2", "pytest-docker~=1.0.0", "pytest-mock~=3.6.1", "docker-compose"] | ||
| TEST_REQUIREMENTS = ["requests-mock~=1.9.3", "pytest~=6.2", "pytest-docker~=2.0", "pytest-mock~=3.6.1"] |
There was a problem hiding this comment.
docker-compose is deprecated and was causing infinite loop when installing test dependencies. We now bump pytest-docker to v2, which removes usages of docker-compose in favor of the new docker compose.
|
| Step | Result |
|---|---|
| Connector package install | ✅ |
| Build source-file docker image for platform(s) linux/x86_64 | ✅ |
| Unit tests | ✅ |
| Integration tests | ✅ |
| Acceptance tests | ✅ |
| Code format checks | ✅ |
| Validate metadata for source-file | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ❌ |
| QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-file test
| @@ -82,6 +82,7 @@ def with_testing_dependencies(context: PipelineContext) -> Container: | |||
|
|
|||
| return ( | |||
| python_environment.with_exec(["pip", "install"] + CONNECTOR_TESTING_REQUIREMENTS) | |||
| .with_exec(sh_dash_c(["curl -fsSL https://get.docker.com | sh"])) | |||
There was a problem hiding this comment.
FYI: In #30474 this with_testing_dependencies function is removed. Connectors test will run inside the connector container on top of which we'd install its testing dependencies according to its setup.py.
I believe docker as a system dependency should not be added to this function, nor to a future "pre/post" hook for source-file.
If we need docker-compose for testing I suggest to install the pip distributed docker-compose by declaring pytest-docker[docker-compose-v1] as a test requirement. see https://github.com/avast/pytest-docker .
If we really want to use the latest docker-compose version I suggest to only install the docker-compose plugin. This can be added to with_testing_dependencies in this PR but eventually added to a pre/post build hook following #30474 .
I'd prefer that we stick to the pip distributed docker compose as it will not require a pre/post build hook implementation for this connector. And I believe it's fine to use an old docker compose version in this context.
What
split from #30984
Integration tests have been broken for source-file for a long time. Installing test dependencies ran indefinitely due to a resolution issue. This PR fixes it.
How