General maintenance and tidying to ensure tests pass. #136
Conversation
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 80.61% 85.26% +4.65%
==========================================
Files 20 24 +4
Lines 459 631 +172
Branches 34 59 +25
==========================================
+ Hits 370 538 +168
- Misses 70 75 +5
+ Partials 19 18 -1
Continue to review full report at Codecov.
|
tillahoffmann
force-pushed
the
m1
branch
3 times, most recently
from
b2e9277 to
4e10204
Compare
tillahoffmann
changed the title
|
@yakimka, planning to merge this one soon as it's been lingering for almost a year. It's a bit of a beast (see description) above. |
|
@tillahoffmann any suggestions on my comments? |
|
@yakimka, I can't see any comments. Are they possibly still showing as pending on your end? |
| @@ -35,7 +35,7 @@ def get_bootstrap_server(self): | |||
| port = self.get_exposed_port(self.port_to_expose) | |||
| return '{}:{}'.format(host, port) | |||
|
|
|||
| @wait_container_is_ready() | |||
| @wait_container_is_ready(UnrecognizedBrokerVersion, NoBrokersAvailable, KafkaError, ValueError) | |||
There was a problem hiding this comment.
UnrecognizedBrokerVersion and NoBrokersAvailable is inherited from KafkaError
Therefore, I think there is no need to specify them explicitly
| @@ -23,7 +23,7 @@ def __init__(self, image="redis:latest", port_to_expose=6379): | |||
| self.port_to_expose = port_to_expose | |||
| self.with_exposed_ports(self.port_to_expose) | |||
|
|
|||
| @wait_container_is_ready() | |||
| @wait_container_is_ready(redis.exceptions.ConnectionError) | |||
There was a problem hiding this comment.
if not client.ping():
raise ExceptionWill this work after changing the wait_container_is_ready logic?
| db.with_bind_ports(3306, 32785) | ||
| with db: | ||
| url = db.get_connection_url() | ||
| container = mysql.MySqlContainer("mariadb:10.6.5")\ |
There was a problem hiding this comment.
Maybe?
container = mysql.MySqlContainer(
"mariadb:10.6.5"
).with_bind_ports(3306, 32785).maybe_emulate_amd64()| @@ -1,6 +1,12 @@ | |||
| ARG version=3.8 | |||
| FROM python:${version} | |||
|
|
|||
| WORKDIR /workspace | |||
There was a problem hiding this comment.
WORKDIR /workspace twice?
|
@tillahoffmann oh, sorry, my fault |
|
Hello. But add transient_exceptions tuple is confusing when use some other docker images. |
|
Hey, we can definitely consider adding transient exceptions as an exposed argument. The motivation behind the change was to not "hide" errors when they occur. E.g. an |
This large-ish PR aims to fix a number of challenges that make some of the tests flaky. At a high level:
pyodbctopymssqlbecause the former hasn't proved particularly reliable and also doesn't work onarmplatforms.wait_container_is_readyto accept a list of "transient errors" for which another try should be attempted. Other errors (such asImportError) reraise the exception immediately. This should avoid some confusion such as PostgresContainer timeout when sqlalchemy not installed #137.maybe_emulate_amd64which sets--platform=linux/amd64if the platform isarm64. This is required for cross-architecture testing because some images are only available asamd64.shtobashbecauseshdoesn't support some of the syntax used (namely brackets). The Kafka tests are still a bit unstable, and I've marked them asxfail.reloadin testing).arm64andamd64.@KerstenBreuer, this might be of interest if you want to get involved more with the code base.
@SergeyPirogov, would be great to get your eyes on this.