use TestContainers for MySQL tests #1424

Merged
merged 5 commits into from Dec 5, 2016

Projects

None yet

2 participants

@bsideup
Contributor
bsideup commented Nov 29, 2016 edited

With this PR we can test schema changes, also later we probably want to test different MySQL versions as well - also possible thanks to TestContainers.

It uses an official Zipkin MySQL Docker image by dropping an old schema before we apply a new one.

Sergei Egorov use TestContainers for MySQL tests
1d8dc6f
@adriancole
Contributor

I'll try to help with this, or you can raise a PR on the zipkin-docker repo to make something more configurable. It is a step backwards to rely on a big image even if travis can pull it down quickly enough. I'm often in slow networks and am usually the one maintaining this code.

Weird that testcontainers can't use mariadb.. can this be addressed? I'd like to not introduce any more dependencies that we'd have to track separately.

@adriancole
Contributor

for example, if you need a more powerful user or something in order to drop schema, you could add it here: https://github.com/openzipkin/docker-zipkin/blob/master/mysql/install.sh#L35

In other words, let's try to make this all coherent. We use the same test images in our tests and ad-hoc docker testing. One docker image version... one base layer.

Zipkin's complex enough without having to remember a matrix of driver and docker images :P

-- all this said, I appreciate that if this were a project that didn't publish its own images for reasons described, I'd also be tempted to use whatever's the standard image was.

@bsideup
Contributor
bsideup commented Nov 30, 2016 edited

Weird that testcontainers can't use mariadb

It can :) There is mariadb container as well. Just if we use MySQLContainer it will attempt to use MySQL driver to check if it's started. I can remove that, for sure

One docker image version... one base layer.

Ok, I'll attempt to use Zipkin's image one more time, shouldn't be that hard after all :)

@adriancole
Contributor
Sergei Egorov added some commits Dec 4, 2016
Sergei Egorov use Zipkin's MySQL image
0ca4785
Sergei Egorov fix zipkin-storage/mysql/pom.xml
1ec0e2c
Sergei Egorov cleanups and code style
f1be294
@adriancole
Contributor

great! looks like it worked fine. 2 small things then merge time!

  • change description of the PR to reflect today :)
  • unhook circleci as I think that's still running the old mysql manually
@bsideup
Contributor
bsideup commented Dec 5, 2016 edited

@adriancole

change description of the PR to reflect today :)

done

unhook circleci as I think that's still running the old mysql manually

I didn't touch CircleCI config in both Elasticsearch and MySQL PRs because I was thinking that we want to keep (for now) one of the CI services to use old testing approach without Docker, no?

@adriancole
Contributor

thanks for checking me. yeah, let's keep them, though we should probably add a comment to the circle ci reminding us that we aren't using docker for services intentionally (to ensure docker-free tests work)

@bsideup
Contributor
bsideup commented Dec 5, 2016

@adriancole well, lack of - docker service record is a good reminder I guess :D But ok, I can make it explicit :)

@adriancole
Contributor

yeah just a reminder of why we aren't adding docker in case we forget.

IIRC we are not yet relying completely on it, nor ask those developing local to always have docker installed. By running on circle w/o docker we have a backup (in case we goofed in our new tests) and also implicitly test the non-docker way.

At some point we may yank the non-docker way of course.

Sergei Egorov add a note to CircleCI config that we don't use Docker-based testing …
…there
8b13f65
@bsideup
Contributor
bsideup commented Dec 5, 2016

@adriancole added :)

@adriancole adriancole merged commit 30d77be into openzipkin:master Dec 5, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adriancole
Contributor

thanks again!

@bsideup
Contributor
bsideup commented Dec 5, 2016

👍

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