jdaugherty commented on code in PR #15067:
URL: https://github.com/apache/grails-core/pull/15067#discussion_r2372364305
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/GrailsGebSettings.groovy:
##########
@@ -65,17 +73,48 @@ class GrailsGebSettings {
recordingFormat = VncRecordingFormat.valueOf(
System.getProperty('grails.geb.recording.format',
DEFAULT_RECORDING_FORMAT.name())
)
- restartRecordingContainerPerTest =
Boolean.parseBoolean(System.getProperty('grails.geb.recording.restartRecordingContainerPerTest',
'true'))
+ restartRecordingContainerPerTest = getBooleanProperty(
+ 'grails.geb.recording.restartRecordingContainerPerTest',
+ true
+ )
implicitlyWait = getIntProperty('grails.geb.timeouts.implicitlyWait',
DEFAULT_TIMEOUT_IMPLICITLY_WAIT)
pageLoadTimeout = getIntProperty('grails.geb.timeouts.pageLoad',
DEFAULT_TIMEOUT_PAGE_LOAD)
scriptTimeout = getIntProperty('grails.geb.timeouts.script',
DEFAULT_TIMEOUT_SCRIPT)
+ atCheckWaiting = getBooleanProperty('grails.geb.atCheckWaiting',
DEFAULT_AT_CHECK_WAITING)
+ timeout = getNumberProperty('grails.geb.timeouts.timeout',
Wait.DEFAULT_TIMEOUT)
+ retryInterval = getNumberProperty('grails.geb.timeouts.retryInterval',
Wait.DEFAULT_RETRY_INTERVAL)
this.startTime = startTime
}
+ private static boolean getBooleanProperty(String propertyName, boolean
defaultValue) {
+ Boolean.parseBoolean(System.getProperty(propertyName,
defaultValue.toString()))
+ }
+
private static int getIntProperty(String propertyName, int defaultValue) {
Integer.getInteger(propertyName, defaultValue) ?: defaultValue
}
+ private static Number getNumberProperty(String propertyName, Number
defaultValue) {
+ def propValue = System.getProperty(propertyName)
+ if (propValue) {
+ try {
+ if (propValue.contains('.')) {
+ return Double.parseDouble(propValue)
Review Comment:
Does the geb api really use Doubles and not BigDecimals?
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -270,58 +367,117 @@ class WebDriverContainerHolder {
Class<? extends ContainerFileDetector> fileDetector
WebDriverContainerConfiguration(SpecInfo spec) {
- ContainerGebConfiguration configuration
+
+ ContainerGebConfiguration conf
// Check if the class implements the interface
if (IContainerGebConfiguration.isAssignableFrom(spec.reflection)) {
- configuration = spec.reflection.getConstructor().newInstance()
as ContainerGebConfiguration
+ conf = spec.reflection.getConstructor().newInstance() as
ContainerGebConfiguration
} else {
// Check for the annotation
- configuration = spec.annotations.find {
+ conf = spec.annotations.find {
it.annotationType() == ContainerGebConfiguration
} as ContainerGebConfiguration
}
- protocol = configuration?.protocol() ?:
ContainerGebConfiguration.DEFAULT_PROTOCOL
- hostName = configuration?.hostName() ?:
ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
- reporting = configuration?.reporting() ?: false
- fileDetector = configuration?.fileDetector() ?:
ContainerGebConfiguration.DEFAULT_FILE_DETECTOR
+ protocol = conf?.protocol() ?:
ContainerGebConfiguration.DEFAULT_PROTOCOL
+ hostName = conf?.hostName() ?:
ContainerGebConfiguration.DEFAULT_HOSTNAME_FROM_CONTAINER
+ reporting = conf?.reporting() ?: false
+ fileDetector = conf?.fileDetector() ?:
ContainerGebConfiguration.DEFAULT_FILE_DETECTOR
}
}
/**
* Workaround for
https://github.com/testcontainers/testcontainers-java/issues/3998
- * Restarts the VNC recording container to enable separate recording files
for each test method.
- * This method uses reflection to access the VNC recording container field
in BrowserWebDriverContainer.
- * Should be called BEFORE each test starts.
+ * <p>
+ * Restarts the VNC recording container to enable separate recording files
for each
+ * test method. This method uses reflection to access the VNC recording
container
+ * field in BrowserWebDriverContainer. Should be called BEFORE each test
starts.
*/
void restartVncRecordingContainer() {
- if (!grailsGebSettings.recordingEnabled ||
!grailsGebSettings.restartRecordingContainerPerTest || !currentContainer) {
+ if (!settings.recordingEnabled ||
!settings.restartRecordingContainerPerTest || !container) {
return
}
try {
// Use reflection to access the VNC recording container field
- Field vncRecordingContainerField =
BrowserWebDriverContainer.getDeclaredField('vncRecordingContainer')
- vncRecordingContainerField.setAccessible(true)
-
- VncRecordingContainer vncContainer =
vncRecordingContainerField.get(currentContainer) as VncRecordingContainer
+ def field =
BrowserWebDriverContainer.getDeclaredField('vncRecordingContainer').tap {
+ accessible = true
+ }
+ def vncContainer = field.get(container) as VncRecordingContainer
if (vncContainer) {
// Stop the current VNC recording container
vncContainer.stop()
// Create and start a new VNC recording container for the next
test
- VncRecordingContainer newVncContainer = new
VncRecordingContainer(currentContainer)
+ def newVncContainer = new VncRecordingContainer(container)
.withVncPassword('secret')
.withVncPort(5900)
- .withVideoFormat(grailsGebSettings.recordingFormat)
- vncRecordingContainerField.set(currentContainer,
newVncContainer)
+ .withVideoFormat(settings.recordingFormat)
+ field.set(container, newVncContainer)
newVncContainer.start()
log.debug('Successfully restarted VNC recording container')
}
} catch (Exception e) {
- log.warn("Failed to restart VNC recording container:
${e.message}", e)
+ log.warn("Failed to restart VNC recording container: $e.message",
e)
// Don't throw the exception to avoid breaking the test execution
}
}
+
+ @CompileStatic
+ private static class ClosureDecorators {
+
+ /**
+ * Wraps a closure so that during its execution,
System.getProperty(key)
+ * returns a custom value instead of what is actually in the system
properties.
+ */
+ static Closure withSystemProperty(Closure target, String key, Object
value) {
+ Closure wrapped = { Object... args ->
+ SysPropScope.withProperty(key, value.toString()) {
+ InvokerHelper.invokeClosure(target, args)
+ }
+ }
+
+ // keep original closure semantics
+ wrapped.rehydrate(target.delegate, target.owner,
target.thisObject).tap {
+ resolveStrategy = target.resolveStrategy
+ }
+ }
+
+ @CompileStatic
+ private static class SysPropScope {
+
+ private static final ThreadLocal<Map<String,String>>
OVERRIDDEN_SYSTEM_PROPERTIES =
+ ThreadLocal.withInitial { [:] as Map<String,String> }
+
+ @Lazy // Thread-safe wrapping of system properties
+ private static Properties propertiesWrappedOnFirstAccess = {
+ new InterceptingProperties().tap {
+ putAll(System.getProperties())
+ System.setProperties(it)
+ }
+ }()
+
+ static <T> T withProperty(String key, String value, Closure<T>
body) {
+ propertiesWrappedOnFirstAccess // Access property to trigger
property wrapping
+ def map = OVERRIDDEN_SYSTEM_PROPERTIES.get()
+ def prev = map.put(key, value)
+ try {
+ return body.call()
+ } finally {
+ if (prev == null) map.remove(key) else map[key] = prev
+ if (map.isEmpty()) OVERRIDDEN_SYSTEM_PROPERTIES.remove()
Review Comment:
Should this be removed? What recreates the map? Is the assumption the
thread won't be reused and will always terminate?
##########
grails-geb/README.md:
##########
@@ -161,6 +177,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.
Review Comment:
I assume this GebConfig is optional?
##########
.github/workflows/gradle.yml:
##########
@@ -173,6 +173,9 @@ jobs:
with:
develocity-access-key: ${{ secrets.GRAILS_DEVELOCITY_ACCESS_KEY }}
- name: "🏃 Run Functional Tests"
+ env:
+ # Make Geb tests more resilient in slow CI environments
+ JAVA_TOOL_OPTIONS: -Dgrails.geb.atCheckWaiting.enabled=true
Review Comment:
Why make CI work differently then locally? Does our JDK read this by
default? I would have assumed we would have defined this property as part of
the test functional test configuration instead.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
