You are viewing a plain text version of this content. The canonical link for it is here.
Posted to geb-dev@groovy.apache.org by "jonnybot0 (via GitHub)" <gi...@apache.org> on 2025/10/01 14:36:42 UTC
[PR] Return global or per thread cache from clearCache method [groovy-geb]
jonnybot0 opened a new pull request, #289:
URL: https://github.com/apache/groovy-geb/pull/289
This is meant to address the problems highlighted in https://github.com/apache/groovy-geb/issues/288. These started after https://github.com/apache/groovy-geb/pull/270, so I'd appreciate a review from @Vampire on this, since he has a lot of context on those changes and can speak to whether this would introduce new problems.
--
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-dev-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3440176963
I'll be honest, I feel ill-equipped to adjudicate this. In my own usage of Geb I've almost exclusively relied on the implicit driver management and not had to worry about the cache. As such, I don't have intuition to rely on when judging how consumers might be using it. For that, @cbmarcum and @Vampire are really the only testimony I have to go on.
I think that returning the driver from this method created a contract that we essentially violated in PR https://github.com/apache/groovy-geb/pull/270.
Based on the [described behavior](https://groovy.apache.org/geb/manual/current/#implicit-driver-management) in the docs that @Vampire pointed to, I think perhaps the best course is to revert #270 and cut an 8.0.1 release. Then, have another go at fixing the problem #270 was meant to address in some better way.
So, order would look like:
1. Merge https://github.com/apache/groovy-geb/pull/291
2. Revert #270
3. Cut an 8.0.1 release (which is being asked for in [other quarters](https://github.com/apache/groovy-geb/pull/285#issuecomment-3427340807))
4. Go to the mailing list / back to the drawing board for a better way to address #270 that doesn't introduce the new bug.
5. Merge that fix and head forward with an 8.0.2 release.
Since you're most impacted, would that path work for both you, @Vampire and you @cbmarcum? Obviously, @Vampire could continue to use 8.0.0 until we could get 8.0.2 out, but folks like @cbmarcum impacted by this problem would be able to use 8.0.1.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3378999775
"Maybe we could enforce global or per-thread caching and not allow changes? Maybe a good topic for the mailing list."
This probably wont work as we have tests that use both ways in the same 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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "Vampire (via GitHub)" <gi...@apache.org>.
Vampire commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3374615153
With that also my initial fix could probably be reverted.
But I don't know whether that is a good idea, as it removes existing functionality, so projects that need both would need to split their tests into two test sets? 🤷♂️
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3448932601
After opening the PR #292 for this branch it reported an error in `module/geb-junit5/src/test/groovy/geb/junit5/GebReportingTestReportingTest.groovy`. After some investigation I found it wasn't caused by my test because when testing the `bugfix/288-driver-cache` branch that test was randomly failing but it did not fail in master after 20 test runs.
When testing 3 sets of 10 runs by: `:module:geb-junit5:test`
Failed run:
1,2,4,9
Succeeded run:
3,5,6,7,8,10
----
Failed run:
1,6,8,9,10
Succeeded run:
2,3,4,5,7
----
Failed run:
1,6,9
Succeeded run:
2,3,4,5,7,8,10
Almost always it was failing on `reportsAreWrittenAtTheEndOfEachTestWhenConfigured` but occasionally it would fail on `canWriteReportsWithTestDerivedName`.
From what I can tell it's not failing at the assertion. It's failing in the runSuccessfuly method.
```
assert listener.wasExecutionSuccessful()
| |
| false
geb.junit5.fixture.TestExecutionResult@5bdc23a1
at org.codehaus.groovy.runtime.InvokerHelper.createAssertError(InvokerHelper.java:414)
at geb.junit5.fixture.TestRunner.runSuccessfully(TestRunner.groovy:30)
at org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:321)
at geb.junit5.GebReportingTestReportingTest.runSuccessfully(GebReportingTestReportingTest.groovy:107)
at org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:321)
at geb.junit5.GebReportingTestReportingTest.reportsAreWrittenAtTheEndOfEachTestWhenConfigured(GebReportingTestReportingTest.groovy:55)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
```
Isolating just this test class runs just fine.
``:module:geb-junit5:test --tests "geb.junit5.GebReportingTestReportingTest"
No issues after 20 runs.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "Vampire (via GitHub)" <gi...@apache.org>.
Vampire commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3440212158
> Since you're most impacted, would that path work for both you
Am I?
I'm not impacted in any major way.
You / the Geb project is impacted most.
Fixing this problem was part of getting rid of the flaky tests in the Geb build, so #188.
I was just investigating and fixing that to help, I did not hit this problem somewhere else, except when contributing to Geb and CI was flaky, as I'm not switching the setting back and forth otherwise.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3444978226
Good points, all around. Given that, how about this plan:
1. Merge @cbmarcum's test from https://github.com/apache/groovy-geb/pull/292 into this branch
2. Merge this PR as a "better than it was" fix
3. Cut an 8.0.1 release
4. Take to the mailing list to talk about how folks are using the manual cache control and see if we can come up with a more durable solution. If we can't, we can just document the known problems in a code comment (use at your own risk, etc. etc.) and move on with life. :)
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3697633677
I've been over this quite a bit. At this point, I think that the split caches is the wrong approach, and does not seem to solve the problem we meant to solve originally (namely, that the `ParallelExecutionSpec` was not reliable). As such, I'm closing this PR and opening https://github.com/apache/groovy-geb/pull/299. I think we need to discern a better fix for the original problem that PR https://github.com/apache/groovy-geb/pull/270 was trying to solve.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "Vampire (via GitHub)" <gi...@apache.org>.
Vampire commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3357285999
Argh, damn, seems we both missed that the driver is returned there, sorry.
Seems also like some tests should be added.
The "fix" you do here is flaky actually.
The original problem that I fixed by splitting that cache into two fields was, if configuration gets changed.
And the same flakiness is in the change you added here.
You still clear both caches, but you return the driver from the global cache if one was found or otherwise the one from the thread-local cache otherwise.
That means that if you changed the `cacheDriverPerThread` configuration at some point, you might even have entries in both and then always return the global one, no matter what the config currently is.
And even if the config would be considered (which probably cannot really be done here), you still would only get one of the two drivers back.
The main question probably is, what is the use-case for returning the driver there to consider what the appropriate options are.
You could consider not returning the driver from that method (having a private variant for the clear-and-quit or just doing the clear there copied.
Or you could consider returning a spreading driver wrapper that contains both drivers if present and each call is fanned out to both if found.
The changes here should work properly as long as the option `cacheDriverPerThread` is not changed after a driver was created, so probably in most consumers out there, but nevertheless the race condition does exist.
Actually, even the `clearCacheAndQuitDriver` is flaky still even with the changes here, as it will only quit the global driver if in both, one was found.
With the spreading wrapper or with copying the clear code and quit both drivers this would be "fixed".
But then it also needs to be defined what actually should happen on `clearCache` and `clearCacheAndQuitDriver`.
Should the latter consider the current configuration and if so, what **is** the current configuration in that static context?
Should both operate on both if both are found?
What should both return?
These API were actually broken since long time ago when the thread-local cache was added in the case of config change.
My changes now just surfaced that there is also a conceptual problem in these cases. :-/
Btw. no, these methods are not just for for testing purposes as the reporter of the current problems assumed. They are documented ways to force Geb to create a new driver on next occasion: https://groovy.apache.org/geb/manual/current/#implicit-driver-management. It is just now a matter of interpretation of the documentation how these methods should work and what they should return if anything. Maybe it would indeed be best to just not return anything from those methods. :-/
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 closed pull request #289: Return global or per thread cache from clearCache method
URL: https://github.com/apache/groovy-geb/pull/289
--
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-dev-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3374628721
Well it is two different configurations in a way. I think a lot broke back in v6 when the configuration per test was done away with.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "jonnybot0 (via GitHub)" <gi...@apache.org>.
jonnybot0 commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3518951092
I was still feeling a bit unsure about this, so I've kicked off the 8.0.1 release without it. Having another look now.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3444486156
@jonnybot0, I do not have any cases where I change the per thread cache during tests at all. Like you, I use implicit caching. #270 fixed flaky test case issues which is good and this PR fixes the issue with that.
It seems the main issue is when users are changing that configuration setting during a run (does anybody?). Unless @Vampire would rather have #270 reverted, I would go with this fix and prefer documentation that users beware changing that configuration is unpredictable. (I could help with that).
Reverting #270 also does not affect my uses but I would rather not have flaky tests :)
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3445045573
> Good points, all around. Given that, how about this plan:
>
> 1. Merge @cbmarcum's test from [test to confirm driver is quit #292](https://github.com/apache/groovy-geb/pull/292) into this branch
>
> 2. Merge this PR as a "better than it was" fix
>
> 3. Cut an 8.0.1 release
>
> 4. Take to the mailing list to talk about how folks are using the manual cache control and see if we can come up with a more durable solution. If we can't, we can just document the known problems in a code comment (use at your own risk, etc. etc.) and move on with life. :)
+1 from me.
--
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] Return global or per thread cache from clearCache method [groovy-geb]
Posted by "cbmarcum (via GitHub)" <gi...@apache.org>.
cbmarcum commented on PR #289:
URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3374606260
I tried using CachingDriverFactory.clearCacheAndQuitDriver() in a cleanup() and with v8.0.0 the old browsers ran until the JVM shutdown. With v7 I never even got a new browser on the next test so v8 was definitely an improvement.
I tested this PR and it seems to fix the issue.
The question how to handle changes in configuration during a JVM is bigger topic I think.
Maybe we could enforce global or per-thread caching and not allow changes? Maybe a good topic for the mailing list.
--
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