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

Support TestContainers PoC #4627

Merged
merged 6 commits into from
Oct 16, 2024
Merged

Support TestContainers PoC #4627

merged 6 commits into from
Oct 16, 2024

Conversation

jamlo
Copy link
Contributor

@jamlo jamlo commented Oct 15, 2024

This PR adds a test suite utilizing TestContainers. It support spinning up docker compose stacks with all bacalhau components, thus resembling production setup.

This is not a complete TestSuite, but a big initial step towards consolidating out testing and evaluating TestContainers usage.

  1. Allow us to run integration tests against a docker compose
    deployment of bacalhau

  2. Bacalhau compute/requestor configuration can be injected in the
    container before start. We do not need to rebuild it every time

  3. Tests are separated into suites which allows the combination of
    similar tests together that will use the same docker compose
    deployment. Help speed up things

  4. Docker compose deployment has a local registry as well as a minio
    server running. This expands out testing ability and scope.

  5. Test Suite Works with Github Actions

Linear issue tracker: ENG-263
Github Issue: #4597

jamlo added 3 commits October 15, 2024 16:39
1- Allow us to run integration tests against a docker compose
   deployment of bacalhau

2- Bacalhau compute/requestor configuration can be injected in the
   container before start. We do not need to rebuild it every time

3- Tests are separated into suites which allows the combination of
   similar tests together that will use the same docker compose
   deployment. Help speed up things

4- Docker compose deployment has a local registry as well as a minio
   server running. This expands out testing ability and scope.

5- Test Suite Works with Github Actions

Linear issue tracker: ENG-263
Github Issue: #4597
@jamlo jamlo marked this pull request as ready for review October 15, 2024 21:59
@jamlo jamlo requested review from wdbaruni and frrist October 15, 2024 21:59
Copy link
Member

@wdbaruni wdbaruni left a comment

Choose a reason for hiding this comment

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

Looks good overall. Nice work! Though taking 4 minutes to run just two scenarios seem too long and I hope we can improve things in upcoming iterations

.github/workflows/testcontainers-integration-tests.yml Outdated Show resolved Hide resolved
jobID, err := extractJobIDFromShortOutput(result)
s.Require().NoErrorf(err, "error extracting Job ID after running it: %q", err)

completedIn, err := s.waitForJobToComplete(jobID, 30*time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

will be great to reuse the state resolvers we already have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it while developing the test suite, my approach was to avoid using any internal Bacalhau packages in this suite to prevent leaking internal implementation details into these tests. Currently, the state resolvers are importing packages from within the main Bacalhau codebase. This concern was one of the primary reasons I introduced test_integration as a separate Go module, establishing a clear boundary. This approach effectively treats the entire Bacalhau deployment as a black box.

With that said, I will be reviewing the state resolvers and see what I can extrtact from them to help us with the tests.

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

LGTM

test_integration/base_suite_test.go Show resolved Hide resolved
test_integration/base_suite_test.go Show resolved Hide resolved
test_integration/base_suite_test.go Outdated Show resolved Hide resolved
test_integration/base_suite_test.go Outdated Show resolved Hide resolved
test_integration/test_utils.go Show resolved Hide resolved
test_integration/test_utils.go Outdated Show resolved Hide resolved
test_integration/test_utils.go Show resolved Hide resolved
test_integration/README.md Outdated Show resolved Hide resolved
test_integration/base_suite_test.go Show resolved Hide resolved
test_integration/go.mod Show resolved Hide resolved
@jamlo
Copy link
Contributor Author

jamlo commented Oct 16, 2024

Looks good overall. Nice work! Though taking 4 minutes to run just two scenarios seem too long and I hope we can improve things in upcoming iterations

@wdbaruni Thank you.
I agree that the timing needs to be significantly lowered. Currently, these are the steps that occur when the suite runs:

  1. The bacalhau project is being compiled (1 to 2 minutes). This is a temporary step that will be totally removed when we choose our CI system, which will have a build step before these tests run, and the binary will be passed down. The integraion tests will not be compiling their own bacalhau binary.

  2. The container base images build: This step is only done one in the whole test, but still I am thinking about a way to use prebuilt container images, pull them from an image registry, and only inject them with tests assets (bacalhau binary, other specs) when the container starts. Thus reducing the overall time.

  3. Each Test suite run is designed to run against a dedicated doeck compose stack, in series. This way if we have 20 tests in a suite, they will be using the same docker compose deployment, and not standing 20 docker compose stacks. This will show more as we add more tests.

@jamlo jamlo merged commit b45a2e0 into main Oct 16, 2024
6 checks passed
@jamlo jamlo deleted the jamlo/test-containers-poc branch October 16, 2024 18:40
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.

3 participants