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

source-mysql: do not use testcontainers in integration tests #30032

Closed
wants to merge 9 commits into from

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Aug 31, 2023

Original problems:

Using testcontainers within airbyte-ci and locally has proven to be complicated:

  • testcontainers used an "old"mysql image which did not have an ARM64 variant. Updating testcontainers solves the problem.
  • In airbyte-ci testcontainers containers and source-mysql containers are running on a sidecar docker host which is not the same host as where the test suite is executed. It means that client code (the tests) can reach the mysql db container using the sidecar dockerhost hostname, but source-mysql can't resolve this host name because it's not running in host network mode: it has to reach the mysql container using its IP. It introduces additional networking challenges in intregration tests, when the connector container has to communicate with the db connector. The difference with local execution of the integration test is that in local execution the docker host == localhost.
  • testcontainers spawns a lot of ephemeral database containers. It is resource intensive and makes running local / airbyte-ci hard.
  • The source-mysql integration test suite has a lot of duplicated code, which makes it hard to understand the test flow and context.

Suggested approach

  • Discard the use of testcontainers for integration test: use a real remote DB provisionned on CloudSQL (GCP)

Leave the the campground cleaner than you found it

  • Refactor the test suite to reduce the duplicated code
  • Improve the gradle caching logic in airbyte-ci to speed up gradle tasks:
    • Enable local build cache when running within airbyte-ci
    • Make gradle container persist their /root/.gradle/ directory to a cache volume

Open questions for java source developers

  • Shall we continue to consider the JdbcSourceAcceptanceTest test suite as part of the unit test? Are you running this test suite locally when iterating on a connector? This test suite uses testcontainers. It has no networking issue while running in airbyte-ci because these tests are not launching a connector as a container but use the MySqlSource object that can directly connect . But as, I mentioned above, using testcontainers adds a lot of overhead to test execution, can we use the remote test instance?

Current blockers

**The java integration tests are hanging. When running airbyte-ci connectors --name=source-mysql test everything runs fine until the integration tests. Then the integration tests starts but the execution of ./gradlew --no-daemon --scan --build-cache --debug :airbyte-integrations:connectors:source-mysql:integrationTest -x airbyteDocker is not returning any progress while eating a ton of CPU. **

TODO

  • Fix failing state related tests
  • Propagate the same approach to source-mysql-strict-encrypt
  • Make the gradle-cache volume persisted by Dagger's magicache to benefit from it in the CI.

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Aug 31, 2023
@github-actions
Copy link
Contributor

Before Merging a Connector Pull Request

Wow! 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:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

.put(JdbcUtils.HOST_KEY, innerContainerAddress.left)
.put(JdbcUtils.PORT_KEY, innerContainerAddress.right)
.put(JdbcUtils.HOST_KEY, HostPortResolver.resolveHost(container))
.put(JdbcUtils.PORT_KEY, HostPortResolver.resolvePort(container))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still work when running directly on macOS with gradle :integrationTestJava?

@alafanechere alafanechere force-pushed the augustin/fix-source-mysql-testing branch 4 times, most recently from 257ac69 to 4843c27 Compare September 4, 2023 07:28
@alafanechere alafanechere force-pushed the augustin/fix-source-mysql-testing branch from aafe73c to 36b6164 Compare September 5, 2023 09:48
@alafanechere alafanechere force-pushed the augustin/fix-source-mysql-testing branch from e4209fb to 693dc87 Compare September 5, 2023 11:08
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Sep 5, 2023
@alafanechere alafanechere changed the title source-mysql: use host port resolver to get the host and port of test containers source-mysql: do not use testcontainers in integration tests Sep 5, 2023
@alafanechere alafanechere marked this pull request as draft September 5, 2023 11:35
@alafanechere alafanechere marked this pull request as ready for review September 6, 2023 12:23
@alafanechere
Copy link
Contributor Author

alafanechere commented Sep 12, 2023

/legacy-test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/6162111976
❌ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/6162111976
🐛 https://gradle.com/s/evsofqsltpcrk

Build Failed

Test summary info:

Could not find result summary

@alafanechere
Copy link
Contributor Author

Closing as this is was fixed in other PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants