You are viewing a plain text version of this content. The canonical link for it is here.
Posted to geb-notifications@groovy.apache.org by "cbmarcum (via GitHub)" <gi...@apache.org> on 2026/03/09 19:42:43 UTC
[PR] Task/geb testcontainers [groovy-geb]
cbmarcum opened a new pull request, #310:
URL: https://github.com/apache/groovy-geb/pull/310
Testcontainers integration for review.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3034673227
##########
gradle/libs.versions.toml:
##########
@@ -63,11 +65,16 @@ sauceConnect = { module = 'com.saucelabs:ci-sauce', version.ref = 'sauceConnect'
selenium-api = { module = 'org.seleniumhq.selenium:selenium-api', version.ref = 'selenium' }
selenium-grid = { module = 'org.seleniumhq.selenium:selenium-grid', version.ref = 'selenium' }
selenium-support = { module = 'org.seleniumhq.selenium:selenium-support', version.ref = 'selenium' }
+selenium-chrome-driver = { module = "org.seleniumhq.selenium:selenium-chrome-driver", version.ref = "selenium" }
+selenium-firefox-driver = { module = "org.seleniumhq.selenium:selenium-firefox-driver", version.ref = "selenium" }
+selenium-edge-driver = { module = "org.seleniumhq.selenium:selenium-edge-driver", version.ref = "selenium" }
+selenium-remote-driver = { module = "org.seleniumhq.selenium:selenium-remote-driver", version.ref = "remote-driver" }
+testcontainers-selenium = { module = "org.testcontainers:selenium", version.ref = "testcontainers" }
spock-core = { module = 'org.spockframework:spock-core', version.ref = 'spock' }
spockCoreGroovy25 = 'org.spockframework:spock-core:1.3-groovy-2.5'
spockCoreGroovy3 = 'org.spockframework:spock-core:2.3-groovy-3.0'
systemLambda = 'com.github.stefanbirkner:system-lambda:1.2.1'
-testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
+# testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
Review Comment:
I changed this to just edit the line.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2983978413
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/ContainerFileDetectorSpockSpec.groovy:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import geb.testcontainers.serviceloader.ServiceRegistry
+import geb.testcontainers.pages.UploadPage
+import org.openqa.selenium.WebDriverException
+import spock.lang.PendingFeature
+
+/**
+ * Altered copy of {@link ContainerFileDetectorAnnotationSpec}
+ */
+class ContainerFileDetectorSpockSpec extends ContainerGebSpecWithServer {
+
+ def setupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, new UselessContainerFileDetector())
+ }
+
+ def cleanupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, null)
+ }
+
+ @PendingFeature(reason = 'https://github.com/apache/grails-geb/pull/146#issuecomment-2691433277')
Review Comment:
Nitpicky thing, but this and some other references to `grails` seem somewhat out of place. I love our friends on Grails, but I fear this might be a wee bit confusing to new contributors.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3024508396
##########
Dockerfile:
##########
Review Comment:
So, I think that building this Dockerfile is a separate concern from testcontainers integration that got folded into this PR. Can we back that out and review it on its own, separately? We made a decision a while back to use the off-the-shelf Docker containers for most build concerns, rather than publishing our own just for CI. Unless I'm misreading the PR, we don't really need it, the docker-entrypoint.sh, build-in-docker.sh, and so on.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3024550706
##########
buildSrc/src/main/groovy/geb.groovy-module.gradle:
##########
@@ -80,5 +80,7 @@ tasks.named('rat') {
'**/MANIFEST.MF', // TODO: can we remove this?
'**/META-INF/services/**', // TODO: can we remove this?
'**/META-INF/groovy/**', // TODO: can we remove this?
+ '**/integration-test/resources/assets/*.txt', // exclude test artifacts
+ '**/integration-test/resources/static/*.html', // exclude test artifacts
Review Comment:
Maybe just add the ASL notice to the top of the files? I know it's nitpicky, but it wouldn't hurt, right?
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3034677519
##########
buildSrc/src/main/groovy/geb.groovy-module.gradle:
##########
@@ -80,5 +80,7 @@ tasks.named('rat') {
'**/MANIFEST.MF', // TODO: can we remove this?
'**/META-INF/services/**', // TODO: can we remove this?
'**/META-INF/groovy/**', // TODO: can we remove this?
+ '**/integration-test/resources/assets/*.txt', // exclude test artifacts
+ '**/integration-test/resources/static/*.html', // exclude test artifacts
Review Comment:
I removed these Rat excludes and updated the tests that check byte size.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3034675181
##########
doc/manual-snippets/real-browser/src/test/resources/GebConfig.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import org.openqa.selenium.chrome.ChromeOptions
+import org.openqa.selenium.firefox.FirefoxOptions
Review Comment:
I reverted this. I'll put it in a separate PR to make it cleaner.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3024523139
##########
gradle/libs.versions.toml:
##########
@@ -63,11 +65,16 @@ sauceConnect = { module = 'com.saucelabs:ci-sauce', version.ref = 'sauceConnect'
selenium-api = { module = 'org.seleniumhq.selenium:selenium-api', version.ref = 'selenium' }
selenium-grid = { module = 'org.seleniumhq.selenium:selenium-grid', version.ref = 'selenium' }
selenium-support = { module = 'org.seleniumhq.selenium:selenium-support', version.ref = 'selenium' }
+selenium-chrome-driver = { module = "org.seleniumhq.selenium:selenium-chrome-driver", version.ref = "selenium" }
+selenium-firefox-driver = { module = "org.seleniumhq.selenium:selenium-firefox-driver", version.ref = "selenium" }
+selenium-edge-driver = { module = "org.seleniumhq.selenium:selenium-edge-driver", version.ref = "selenium" }
+selenium-remote-driver = { module = "org.seleniumhq.selenium:selenium-remote-driver", version.ref = "remote-driver" }
+testcontainers-selenium = { module = "org.testcontainers:selenium", version.ref = "testcontainers" }
spock-core = { module = 'org.spockframework:spock-core', version.ref = 'spock' }
spockCoreGroovy25 = 'org.spockframework:spock-core:1.3-groovy-2.5'
spockCoreGroovy3 = 'org.spockframework:spock-core:2.3-groovy-3.0'
systemLambda = 'com.github.stefanbirkner:system-lambda:1.2.1'
-testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
+# testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
Review Comment:
Nit: Why this commented out line?
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2991442756
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/PortConfigFromClassSpec.groovy:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import spock.lang.Narrative
+
+/**
+ * Test spec to verify that the correct port configuration is being used.
+ * Some reference taken from:
+ * groovy-geb/module/geb-core/src/test/groovy/geb/conf/ConfigurationLoaderSpec.groovy
+ */
+@Narrative("To verify that class field overrides config file.")
+class PortConfigFromClassSpec extends ContainerGebSpec {
+
+ static TestFileServer server
+
+ int hostPort = 8000
Review Comment:
The possible settings of hostPort are described in the [readme host port section](https://github.com/apache/groovy-geb/tree/task/geb-testcontainers/integration/geb-testcontainers#host-port).
The GebConfig.groovy for this test sets it to 8090 (option 2). This test is meant to prove that setting it with a class level field in the test (Option 3) will override the option 2 configuration.
See the WebDriverContainerHolder findServerPort method.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#issuecomment-4041437178
A question you asked in your mail:
> Should we change the packages from grails.plugin.geb or leave them alone?
I would say we should move the packages since the plan is to incorporate this into geb itself. This won't be a Grails plugin hosted within Geb, but a native integration with testcontainers.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2983719550
##########
integration/geb-testcontainers/geb-testcontainers.gradle:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'geb.test-framework-integration-module'
+}
+
+// tests require java 18+ for SimpleFileServer
+java {
+ toolchain {
+ languageVersion = JavaLanguageVersion.of(21)
+ }
+ sourceCompatibility = JavaVersion.VERSION_11
+ targetCompatibility = JavaVersion.VERSION_11
+}
+
+ext {
+ // The drivers we want to use in containers
+ // see https://github.com/SeleniumHQ/docker-selenium
+ // for browsers per platform
+ // drivers = ["firefox", "chrome", "edge"] // no safari container
+ drivers = ["firefox"]
+}
+
+// add integration tests
+sourceSets {
+ integrationTest {
+ groovy {
+ srcDirs = ['src/integration-test/groovy']
+ }
+ resources {
+ srcDirs = ['src/integration-test/resources']
+ }
+ compileClasspath += sourceSets.main.output
+ runtimeClasspath += sourceSets.main.output
+ }
+}
+
+configurations {
+ integrationTestImplementation.extendsFrom implementation
+ integrationTestRuntimeOnly.extendsFrom runtimeOnly
+}
+
+dependencies {
+ api project(":module:geb-spock")
+ api libs.spock.core
+ api libs.selenium.chrome.driver
+ api libs.selenium.firefox.driver
+ api libs.selenium.edge.driver
+ api libs.selenium.remote.driver
+ api libs.testcontainers.selenium
+
+ implementation libs.selenium.support
+
+
+ testImplementation libs.junit.platform.testkit
+ testRuntimeOnly libs.junit.platform.launcher
+
+ integrationTestRuntimeOnly libs.junit.platform.launcher
+}
+
+
+tasks.withType(Test).configureEach {
+ outputs.upToDateWhen { false } // always run tests
+ useJUnitPlatform()
+}
+
+tasks.named('test') {
+ testLogging {
+ events 'passed', 'skipped', 'failed'
+ }
+
+ systemProperty('geb.container.recording.mode', 'RECORD_ALL') // also RECORD_FAILING
+}
+
+tasks.register('integrationTest', Test) {
Review Comment:
The way this task is wired up seems to prevent someone from running just one test. I think I've fixed that in my feedback PR at https://github.com/apache/groovy-geb/pull/313. @me if that doesn't look right, though.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3028900513
##########
gradle/libs.versions.toml:
##########
@@ -63,11 +65,16 @@ sauceConnect = { module = 'com.saucelabs:ci-sauce', version.ref = 'sauceConnect'
selenium-api = { module = 'org.seleniumhq.selenium:selenium-api', version.ref = 'selenium' }
selenium-grid = { module = 'org.seleniumhq.selenium:selenium-grid', version.ref = 'selenium' }
selenium-support = { module = 'org.seleniumhq.selenium:selenium-support', version.ref = 'selenium' }
+selenium-chrome-driver = { module = "org.seleniumhq.selenium:selenium-chrome-driver", version.ref = "selenium" }
+selenium-firefox-driver = { module = "org.seleniumhq.selenium:selenium-firefox-driver", version.ref = "selenium" }
+selenium-edge-driver = { module = "org.seleniumhq.selenium:selenium-edge-driver", version.ref = "selenium" }
+selenium-remote-driver = { module = "org.seleniumhq.selenium:selenium-remote-driver", version.ref = "remote-driver" }
+testcontainers-selenium = { module = "org.testcontainers:selenium", version.ref = "testcontainers" }
spock-core = { module = 'org.spockframework:spock-core', version.ref = 'spock' }
spockCoreGroovy25 = 'org.spockframework:spock-core:1.3-groovy-2.5'
spockCoreGroovy3 = 'org.spockframework:spock-core:2.3-groovy-3.0'
systemLambda = 'com.github.stefanbirkner:system-lambda:1.2.1'
-testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
+# testcontainers-selenium = 'org.testcontainers:selenium:1.20.4'
Review Comment:
It moved to line 72 and uses a version.ref.. It should have been removed.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3028881289
##########
doc/manual-snippets/real-browser/src/test/resources/GebConfig.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import org.openqa.selenium.chrome.ChromeOptions
+import org.openqa.selenium.firefox.FirefoxOptions
Review Comment:
Sort of. This is a container test and I'm using macOS arm64 and there is no native Chrome arm64 so image they fail for me. That is the reason I changed the default container browser from Chrome to Firefox.
[docker-selenium architectures](https://github.com/SeleniumHQ/docker-selenium?tab=readme-ov-file#browser-images-in-multi-arch).
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3028920565
##########
Dockerfile:
##########
Review Comment:
I've but it back to original. I'll do a separate PR with the explanation for it. You are correct, It is not required for this PR.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2983751153
##########
integration/geb-testcontainers/src/main/groovy/geb/testcontainers/support/delegate/DownloadSupportDelegate.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb.support.delegate
+
+import geb.download.DownloadSupport
+import grails.plugin.geb.ContainerGebSpec
+import groovy.transform.CompileStatic
+import groovy.transform.SelfType
+
+/**
+ * Handles delegation to the DownloadSupport instance so that the Geb API can be used directly in the test.
+ * <p>
+ * As method parameter names are not available in the Geb artifacts we are delegating manually to
+ * get the best possible IDE support and user experience.
+ *
+ * @author Mattias Reichel
+ * @since 4.2
+ */
+@CompileStatic
+@SelfType(ContainerGebSpec)
+trait DownloadSupportDelegate implements DownloadSupport {
Review Comment:
Alrighty. I've got a PR up with a proposed refactor. https://github.com/apache/groovy-geb/pull/313
Let me know what you think. I'm not pretending my approach is more perfect, though it does seem a bit more consistent with the rest of Geb (extending a specific base class).
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2920850523
##########
integration/geb-testcontainers/src/main/groovy/grails/plugin/geb/support/delegate/DownloadSupportDelegate.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb.support.delegate
+
+import geb.download.DownloadSupport
+import grails.plugin.geb.ContainerGebSpec
+import groovy.transform.CompileStatic
+import groovy.transform.SelfType
+
+/**
+ * Handles delegation to the DownloadSupport instance so that the Geb API can be used directly in the test.
+ * <p>
+ * As method parameter names are not available in the Geb artifacts we are delegating manually to
+ * get the best possible IDE support and user experience.
+ *
+ * @author Mattias Reichel
+ * @since 4.2
+ */
+@CompileStatic
+@SelfType(ContainerGebSpec)
+trait DownloadSupportDelegate implements DownloadSupport {
Review Comment:
I suspect that the traits in this package may be something we could simplify and remove for the Geb library. If the Grails test containers integration wants to keep doing things this way, that's totally fine. For the Geb library, my intuition is that we can provide a simpler base class for people to build off.
I recognize, you're probably following this pattern in no small part to provide continuity with the old Grails plugin, so no complaints. This also may be something that's better shown than told. I'm going to try to come up with an alternate implementation and open a PR into your branch to demonstrate what I mean. If I can't get it working, you'll know that it was just me overthinking! 😄
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3006245516
##########
integration/geb-testcontainers/src/main/groovy/geb/testcontainers/support/delegate/DownloadSupportDelegate.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb.support.delegate
+
+import geb.download.DownloadSupport
+import grails.plugin.geb.ContainerGebSpec
+import groovy.transform.CompileStatic
+import groovy.transform.SelfType
+
+/**
+ * Handles delegation to the DownloadSupport instance so that the Geb API can be used directly in the test.
+ * <p>
+ * As method parameter names are not available in the Geb artifacts we are delegating manually to
+ * get the best possible IDE support and user experience.
+ *
+ * @author Mattias Reichel
+ * @since 4.2
+ */
+@CompileStatic
+@SelfType(ContainerGebSpec)
+trait DownloadSupportDelegate implements DownloadSupport {
Review Comment:
@jonnybot0 , I've merged your #313 changes and other cleanups. Please have another go at it.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3024536053
##########
doc/manual-snippets/real-browser/src/test/resources/GebConfig.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import org.openqa.selenium.chrome.ChromeOptions
+import org.openqa.selenium.firefox.FirefoxOptions
Review Comment:
I'm not *against* this change, strictly speaking (happy Firefox user), but it feels a bit unrelated to test-containers.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3006242713
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/ContainerFileDetectorSpockSpec.groovy:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import geb.testcontainers.serviceloader.ServiceRegistry
+import geb.testcontainers.pages.UploadPage
+import org.openqa.selenium.WebDriverException
+import spock.lang.PendingFeature
+
+/**
+ * Altered copy of {@link ContainerFileDetectorAnnotationSpec}
+ */
+class ContainerFileDetectorSpockSpec extends ContainerGebSpecWithServer {
+
+ def setupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, new UselessContainerFileDetector())
+ }
+
+ def cleanupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, null)
+ }
+
+ @PendingFeature(reason = 'https://github.com/apache/grails-geb/pull/146#issuecomment-2691433277')
Review Comment:
not nitpicky at all. I've updated the capability names to geb:* And it seems you have fixed that test's need for pending.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2983745592
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/PortConfigFromClassSpec.groovy:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import spock.lang.Narrative
+
+/**
+ * Test spec to verify that the correct port configuration is being used.
+ * Some reference taken from:
+ * groovy-geb/module/geb-core/src/test/groovy/geb/conf/ConfigurationLoaderSpec.groovy
+ */
+@Narrative("To verify that class field overrides config file.")
+class PortConfigFromClassSpec extends ContainerGebSpec {
+
+ static TestFileServer server
+
+ int hostPort = 8000
Review Comment:
This field seems effectively unused. That makes me wonder what exactly this test is testing. `"should use the class field value"` doesn't seem to describe what the test is doing.
Likewise, this test has been flaky for me locally. At first I thought it was because I had other containers running on that port, but that was not it.
I've pushed what I think is a fix to my review branch, but I'd like to know what the purpose of this test is. Just because it passes doesn't matter if I don't understand what functionality this test is meant to verify.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2935827559
##########
integration/geb-testcontainers/src/main/groovy/geb/testcontainers/support/delegate/DownloadSupportDelegate.groovy:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.plugin.geb.support.delegate
+
+import geb.download.DownloadSupport
+import grails.plugin.geb.ContainerGebSpec
+import groovy.transform.CompileStatic
+import groovy.transform.SelfType
+
+/**
+ * Handles delegation to the DownloadSupport instance so that the Geb API can be used directly in the test.
+ * <p>
+ * As method parameter names are not available in the Geb artifacts we are delegating manually to
+ * get the best possible IDE support and user experience.
+ *
+ * @author Mattias Reichel
+ * @since 4.2
+ */
+@CompileStatic
+@SelfType(ContainerGebSpec)
+trait DownloadSupportDelegate implements DownloadSupport {
Review Comment:
Sure, let me know if you have a chance to look into it.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#issuecomment-4062996788
The packages have been changed to geb.testcontainers consistent with other module conventions.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 merged PR #310:
URL: https://github.com/apache/groovy-geb/pull/310
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2991442756
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/PortConfigFromClassSpec.groovy:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import spock.lang.Narrative
+
+/**
+ * Test spec to verify that the correct port configuration is being used.
+ * Some reference taken from:
+ * groovy-geb/module/geb-core/src/test/groovy/geb/conf/ConfigurationLoaderSpec.groovy
+ */
+@Narrative("To verify that class field overrides config file.")
+class PortConfigFromClassSpec extends ContainerGebSpec {
+
+ static TestFileServer server
+
+ int hostPort = 8000
Review Comment:
The possible settings of hostPort are described in the [readme host port section](https://github.com/apache/groovy-geb/tree/task/geb-testcontainers/integration/geb-testcontainers#host-port).
The GebConfig.groovy for this test sets it to 8090 (option 2). This test is meant to prove that setting it with a class level field in the test (Option 3) will override the option 2 configuration.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r2983978413
##########
integration/geb-testcontainers/src/integration-test/groovy/geb/testcontainers/ContainerFileDetectorSpockSpec.groovy:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package geb.testcontainers
+
+import geb.testcontainers.serviceloader.ServiceRegistry
+import geb.testcontainers.pages.UploadPage
+import org.openqa.selenium.WebDriverException
+import spock.lang.PendingFeature
+
+/**
+ * Altered copy of {@link ContainerFileDetectorAnnotationSpec}
+ */
+class ContainerFileDetectorSpockSpec extends ContainerGebSpecWithServer {
+
+ def setupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, new UselessContainerFileDetector())
+ }
+
+ def cleanupSpec() {
+ ServiceRegistry.setInstance(ContainerFileDetector, null)
+ }
+
+ @PendingFeature(reason = 'https://github.com/apache/grails-geb/pull/146#issuecomment-2691433277')
Review Comment:
Nitpicky thing, but this and some other references to `grails` seem somewhat out of place. Same thing for the `grails:gebConfigUsed` properties used elsewhere. I love our friends on Grails, but I fear this might be a wee bit confusing to new contributors.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on code in PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#discussion_r3028881289
##########
doc/manual-snippets/real-browser/src/test/resources/GebConfig.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import org.openqa.selenium.chrome.ChromeOptions
+import org.openqa.selenium.firefox.FirefoxOptions
Review Comment:
Sort of. This is a container test and I'm using macOS arm64 and there is no native Chrome arm64 image so they fail for me. That is the reason I changed the default container browser from Chrome to Firefox.
[docker-selenium architectures](https://github.com/SeleniumHQ/docker-selenium?tab=readme-ov-file#browser-images-in-multi-arch).
Let me know if you prefer it not to be in this PR.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Task/geb testcontainers [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #310:
URL: https://github.com/apache/groovy-geb/pull/310#issuecomment-4185532124
Hi @jonnybot0 , I think I've handled everything suggested. Please have another look.
--
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: geb-notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org