JonasPammer commented on code in PR #15067:
URL: https://github.com/apache/grails-core/pull/15067#discussion_r2356688532
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -59,99 +64,130 @@ import grails.plugin.geb.serviceloader.ServiceRegistry
class WebDriverContainerHolder {
private static final String DEFAULT_HOSTNAME_FROM_HOST = 'localhost'
+ private static final String REMOTE_ADDRESS_PROPERTY =
'webdriver.remote.server'
+ private static final String[] SELENIUM_BROWSERS = ['chrome', 'edge',
'firefox']
+ private static final String DEFAULT_DOCKER_IMAGE_NAME =
'selenium/standalone-chrome'
- GrailsGebSettings grailsGebSettings
+ GrailsGebSettings settings
GebTestManager testManager
- Browser currentBrowser
- BrowserWebDriverContainer currentContainer
- WebDriverContainerConfiguration currentConfiguration
+ Browser browser
+ BrowserWebDriverContainer container
+ WebDriverContainerConfiguration containerConf
- WebDriverContainerHolder(GrailsGebSettings grailsGebSettings) {
- this.grailsGebSettings = grailsGebSettings
+ WebDriverContainerHolder(GrailsGebSettings settings) {
+ this.settings = settings
}
boolean isInitialized() {
- currentContainer != null
+ container != null
}
void stop() {
- currentContainer?.stop()
- currentContainer = null
- currentBrowser = null
+ container?.stop()
+ container = null
+ browser = null
testManager = null
- currentConfiguration = null
+ containerConf = null
}
- boolean
matchesCurrentContainerConfiguration(WebDriverContainerConfiguration
specConfiguration) {
- specConfiguration == currentConfiguration &&
grailsGebSettings.recordingMode ==
BrowserWebDriverContainer.VncRecordingMode.SKIP
+ boolean
matchesCurrentContainerConfiguration(WebDriverContainerConfiguration specConf) {
+ specConf == containerConf &&
+ settings.recordingMode ==
BrowserWebDriverContainer.VncRecordingMode.SKIP
}
- private static int getPort(IMethodInvocation invocation) {
+ private static int findServerPort(IMethodInvocation methodInvocation) {
try {
- return (int)
invocation.instance.metaClass.getProperty(invocation.instance, 'serverPort')
+ return (int) methodInvocation.instance.metaClass.getProperty(
+ methodInvocation.instance,
+ 'serverPort'
+ )
} catch (ignored) {
- throw new IllegalStateException('Test class must be annotated with
@Integration for serverPort to be injected')
+ throw new IllegalStateException(
+ 'Test class must be annotated with @Integration for
serverPort to be injected'
+ )
}
}
@PackageScope
- boolean reinitialize(IMethodInvocation invocation) {
- WebDriverContainerConfiguration specConfiguration = new
WebDriverContainerConfiguration(
- invocation.getSpec()
+ boolean reinitialize(IMethodInvocation methodInvocation) {
+ def specConf = new WebDriverContainerConfiguration(
+ methodInvocation.spec
)
- if (matchesCurrentContainerConfiguration(specConfiguration)) {
+ if (matchesCurrentContainerConfiguration(specConf)) {
return false
}
if (initialized) {
stop()
}
- currentConfiguration = specConfiguration
- currentContainer = new BrowserWebDriverContainer().withRecordingMode(
- grailsGebSettings.recordingMode,
- grailsGebSettings.recordingDirectory,
- grailsGebSettings.recordingFormat
+ def gebConf = new ConfigurationLoader().conf
+ def dockerImageName = defaultDockerImageName
+
+ def gebConfigFileFound = false
+ if (gebConfigFileExists) {
+ if (!gebConf.driverConf instanceof Closure) {
+ throw new IllegalStateException(
+ 'The "driver" config value must be a Closure that
returns an instance of RemoteWebDriver.'
+ )
+ }
+ gebConfigFileFound = true
+
+ // Prepare for creating a suitable container matching the driver
+ // configured in GebConfig.groovy, or more specifically,
+ // specified by the `configuredBrowser` property.
+ dockerImageName =
createDockerImageName(gebConf.rawConfig.configuredBrowser)
Review Comment:
regression for GebConfig's that don't have this special var - add a default
here?
we already imply Chrome at `new RemoteWebDriver(container.seleniumAddress,
new ChromeOptions()` when no GebConfig existed on classpath
##########
grails-geb/README.md:
##########
@@ -161,6 +161,28 @@ To enable tracing, set the following system property:
This allows you to opt in to tracing when an OpenTelemetry collector is
available.
+#### GebConfig.groovy and using non-default browser settings
+Provide a `GebConfig.groovy` on the test runtime classpath (commonly
`src/integration-test/resources`, but any location on the test classpath works)
to customize the browser.
+
+To make this work, ensure:
+1. The `driver` property in your `GebConfig` is a `Closure` that returns a
`RemoteWebDriver` instance.
+2. You set a custom `configuredBrowser` property so that `ContainerGebSpec`
can start a matching container (e.g. "chrome", "firefox", "edge").
Review Comment:
as pointed out some projects potentially have GebConfig next to
ContainerGebConfig. I saw other extensions always prefix their variables with
their extension (e.g. sauceLabsBrowser), maybe we should too to avoid
confusion? e.g. `containerBrowser` (in reference to containergebspec)
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -245,19 +268,47 @@ class WebDriverContainerHolder {
/**
* Returns the hostname that the server under test is available on from
the host.
- * <p>This is useful when using any of the {@code download*()} methods as
they will connect from the host,
- * and not from within the container.
+ * <p>This is useful when using any of the {@code download*()} methods as
they will
+ * connect from the host, and not from within the container.
+ *
* <p>Defaults to {@code localhost}. If the value returned by {@code
webDriverContainer.getHost()}
- * is different from the default, this method will return the same value
same as {@code webDriverContainer.getHost()}.
+ * is different from the default, this method will return the same value
same as
+ * {@code webDriverContainer.getHost()}.
*
* @return the hostname for accessing the server under test from the host
*/
String getHostNameFromHost() {
- return hostNameChanged ? currentContainer.host :
DEFAULT_HOSTNAME_FROM_HOST
+ hostNameChanged ? container.host : DEFAULT_HOSTNAME_FROM_HOST
}
private boolean isHostNameChanged() {
- return currentContainer.host !=
ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
+ container.host !=
ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
+ }
+
+ private static DockerImageName getDefaultDockerImageName() {
+ DockerImageName.parse("$DEFAULT_DOCKER_IMAGE_NAME:$seleniumVersion")
+ }
+
+ private static DockerImageName createDockerImageName(Object
configuredBrowser) {
+ validateConfiguredBrowser(configuredBrowser)
+
DockerImageName.parse("selenium/standalone-$configuredBrowser:$seleniumVersion")
+ }
+
+ private static void validateConfiguredBrowser(Object browser) {
Review Comment:
why only allow a hardcoded subset?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@grails.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
