Fix docker availability handling accross platforms #139199
Conversation
breskeby
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-delivery (Team:Delivery) |
jozala
left a comment
jozala
left a comment
There was a problem hiding this comment.
I see the DockerAvailabilty part is mostly code moved from DockerEnvironmentAwareTestContainer, but I've got some doubts about how this is expected to work.
I'd like to clarify the CI comment, but I would like to also understand the general aim.
- Do we want to ignore the problematic Docker tests runs in the CI or outside of it?
- I understand it makes sense for the
EXCLUDED_OSin the CI, because these OSes fromdockerOnLinuxExclusionsfile cannot run Docker for whatever reason. However, I think we should fail in CI if we are on a supported OS, but Docker is unavailable (DOCKER_PROBING_SUCCESSFUL == false). With the current implementation, we hide the infrastructure configuration issues and silently ignore the tests whenever Docker is not available.
|
|
||
| static void assumeDockerIsAvailable() { | ||
| org.junit.Assume.assumeFalse("The current OS is excluded from Docker-based tests", EXCLUDED_OS); | ||
| org.junit.Assume.assumeTrue("The current OS is excluded from Docker-based tests", DOCKER_PROBING_SUCCESSFUL); |
There was a problem hiding this comment.
NIT: Should this have a different message so it is easy to distinguish between excluded OS and Docker unavailable?
| if (System.getProperty("os.name").toLowerCase().startsWith("windows")) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Why do we want to completely disable tests Docker-based test on Windows outside of CI?
Is it really that bad with the Testcontainers support on Windows?
| if (CI) { | ||
| // we dont exclude OS outside of CI environment | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I may get lost a bit in the number of negations here, but isn't it the opposite to what the comment says?
From the code, I understand that if CI == true then we do not exclude. That means we never exclude the OS if we are in the CI, but only exclude when running locally.
For the CI run the isExcludedOs() is always false. This value is used in the assumeFalse so if we are in CI we get assumeFalse(false), and the test will always pass this assume invocation.
There was a problem hiding this comment.
totally valid concern. It was actually wrong originally already I think. fixed this.
you are totally correct. I have just moved stuff and refactored but actually the CI handling wasn't clear. So far we have indeed just ignored those tests when docker not available. that actually left us with a hole in our coverage and detecting docker broken on ci is hard. I think your suggestion is right and we should fail in environments that we expect to work. We can then be explicit which OSs to exclude. Overall I think we just don't test docker on windows either as our images don't have docker support. |
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
breskeby
force-pushed
the
fix/docker-windows-availability
branch
from
f26aea7 to
2d3604e
Compare
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
* Fix docker availability handling accross platforms
- Invert logic in isExcludedOs to correctly apply exclusions only in CI.
- Fail tests in CI if Docker is missing on supported OS instead of skipping.
- Update assumption messages for clarity.
3 participants
This addresses some issues detecting docker availability and test skipping on windows after updating testcontainer to > 2.x. This is a fallout from our improvements on rerunning periodic builds and I ran into this repeatedly there