-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Automatically set SHM for selenium container, and allow SHM size to be configured #1751
Conversation
modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add tests that verify both behaviors? The "mounted shm" one can simply assert the result of getContainerInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/selenium/src/test/java/org/testcontainers/junit/BrowserWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
) { | ||
webDriverContainer.start(); | ||
assertEquals("Shared memory size is configured", 512 * FileUtils.ONE_MB, webDriverContainer.getShmSize()); | ||
assertEquals("No mounts present", webDriverContainer.getContainerInfo().getMounts().size(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check that there are no shm
mounts, not just any, so that if we add some volumes in future, it won't break the test (same in createContainerWithShmVolume
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR this one was only partially resolved. @glefloch please do not resolve other's comments, let the author resolve it when he thinks it is so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @bsideup, I will open another pull request with this fix. I thought it was only for this test...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/selenium/src/test/java/org/testcontainers/junit/BrowserWebDriverContainerTest.java
Outdated
Show resolved
Hide resolved
@rnorth I left a couple of comments about the tests, good to go once fixed :) |
close #1670 by setting the shared memory to 2Go.