Initial toolchain upgrade #185
Conversation
|
Thanks! This looks on the right track. We will need to appropriately update the CircleCI configuration for this to work. These errors: Seem to indicate that Gradle isn't detecting the version of Java we need as installed in the docker image it's running on. .circleci/config.yml seems to indicate that we're using a custom image. That image is built from https://github.com/geb/ci-docker-image. I'd bet if we added Let's hold off on merging this until we've got the build steps passing. I'll be happy to merge as many PRs as we need on the CI image. While you're in there, it would be worth it to try and update the Ubuntu image it's based on, since I've already done that for the build tasks that build directly on Ubuntu. |
Poundex
force-pushed
the
topic/initial-toolchain-upgrade
branch
from
9e4cb78 to
795ffb6
Compare
|
Web Driver binaries plugin removed as the Web Drivers are now baked directly into the image (this was necessary for the ChromeDriver anyway due to version incompatibilities with the browser). Updated the CI image and the CI config and now builds are passing. Moved to draft pending getting a new version of the CI image built and pushed. |
Poundex
force-pushed
the
topic/initial-toolchain-upgrade
branch
3 times, most recently
from
9c59964 to
59b1b58
Compare
| - run: | ||
| docker network create dind | ||
| docker run -d --network dind -e DOCKER_TLS_CERTDIR="" --privileged --name dind docker:dind | ||
| sleep 20 |
There was a problem hiding this comment.
20 seconds seems like a long time. Does it need to be that long? What is the sleep for, exactly?
There was a problem hiding this comment.
The delay is waiting for the dind container to start, which includes the container itself waiting for about 15 seconds, so 20 seconds is just to make sure it has had time to fully start up
gradle/libs.versions.toml
Outdated
| sauceConnect = "1.153" | ||
| selenium = "4.2.2" | ||
| spock = "2.3-groovy-4.0" | ||
| spock = "2.4-M4-groovy-4.0" |
There was a problem hiding this comment.
I'd prefer we stick to stable releases when merging to master. Milestones are fine for testing. Is there a reason you need 2.4-M4 over 2.3?
There was a problem hiding this comment.
This is the one bit of feedback I'd prefer to see addressed. Let's use 2.3, and we should be good to go.
| website = "https://gebish.org/manual/current/#gradle-plugins" | ||
| vcsUrl = "https://github.com/geb/geb" | ||
| tags = ["geb", "testing", "cloud", "browser"] |
There was a problem hiding this comment.
We should carry over these tags. Just use
tags.set(['search', 'tags', 'for', 'your', 'hello', 'plugin'])
rather than tags = ...
There was a problem hiding this comment.
Ah, nevermind, I see you've moved them to plugin-definitions.gradle. That's fine.
There was a problem hiding this comment.
Yeah this is just a change to the Gradle plugin plugin that has had to be followed
| super(remoteAddress, new ChromeOptions().addArguments('headless')) | ||
| super(remoteAddress, new ChromeOptions() | ||
| .addArguments('headless') | ||
| .addArguments('--remote-allow-origins=*') |
There was a problem hiding this comment.
I know it's redundant, but could we add the same TODO here as above?
| @@ -18,11 +18,13 @@ package geb.module | |||
| import geb.test.GebSpecWithCallbackServer | |||
| import geb.test.browsers.Chrome | |||
| import geb.test.browsers.RequiresRealBrowser | |||
| import spock.lang.Ignore | |||
|
|
|||
| import java.time.LocalDateTime | |||
|
|
|||
| @Chrome | |||
| @RequiresRealBrowser // maybe due to https://sourceforge.net/p/htmlunit/bugs/1923/ | |||
There was a problem hiding this comment.
Ignoring these tests has me a wee bit nervous that we've broken something. For a major release with a proper deprecation announcement, I'm okay with that, but that's not what we're aiming for here. I'm going to play around with these locally and have a think,.
There was a problem hiding this comment.
I've captured the tests I disabled on #188. I've looked into two out of three of them. One of them is a change in Chrome itself. I've already fixed that one here https://github.com/Poundex/geb/tree/topic/fix-datetime-local-chrome it's just based off the upgraded branch so was waiting for that to be in before opening the MR.
The reporting one I've investigated, and it also seems genuinely broken, but it's to do with threads so sometimes needs to be ran a few times before it can be seen failing (Spock's @RepeatUntilFailure is handy for this). I don't know if this was passing by chance before, or something has changed in the way either Gradle or Spock runs stuff in parallel. I haven't looked into fixing this one yet as it seems like quite a complicated task, but it does appear that Geb's reporting is not thread-safe.
I haven't had a chance to look into the third one yet.
There was a problem hiding this comment.
I couldn't reproduce the issue with PageOrientedSpec so I've enabled it and it seems to be passing in CI
There was a problem hiding this comment.
Thanks! Glad to hear the PageOrientedSpec test is back online.
The other two problems (the date spec and the parallel execution spec) have me more nervous. Suppose someone is passing an ISO8601 formatted date time to Geb. Before this update, that works. After this update, it doesn't. Same problem if they're relying on parallel execution.
In this case, working around it by using JavaScript seems like hiding a breaking change. Here, the test is a set of executable documentation telling us what the value API is expected to do.
I haven't had a chance to really dig into this yet, but I really think we shouldn't rely on workarounds if we can avoid it at all. I think we're on the right path, but ideally the test API would continue to behave the same as before. Tooling updates shouldn't break that.
There was a problem hiding this comment.
I've done a bit more digging into the issues around the DateTimeLocalInputSpec and left a comment on #188. In short, the root cause is the Java update changes the default precision of LocalDateTime.now(), and as the String value of this is the only thing used as the test value then this test was always passing by accident. Whether it be caused by a Java update past 9 which changes the default precision, or be it by a datetime String coming from some other source that has a precision greater than millisecond then there is no combination of Chrome and Geb version where that test would have passed. There should definitely be some discussion on how to approach the problem, but I think that should be a focused conversation in its own thread, and not here.
It is a similar story with the ParallelExecutionSpec, nothing is behaving any differently then it was before these changes and the issue is reproducible in current master. If you don't want to make any releases before these issues are addressed then that's fine, but I think they are outside the scope of this MR.
There was a problem hiding this comment.
That all makes sense. Let's merge for now and carry on discussion over on the issue.
Poundex
force-pushed
the
topic/initial-toolchain-upgrade
branch
2 times, most recently
from
3502e8f to
cd4e319
Compare
Poundex
force-pushed
the
topic/initial-toolchain-upgrade
branch
from
5247b6e to
267eea2
Compare
Here's a start on getting the build and dependencies updated. I've updated Gradle to 8, and it will now build with Java 11.
The full build of the Geb components, all the test suites, and the docsites & manuals are all building & passing for me locally. Most of it is also working in Geb's CI container image, except for a few of the documentation-based tasks. I don't know if they would have actually failed in CI or it is an artefact of my local build environment. That/those images will need updating with a newer JDK, anyway.
Did a very brief smoke test with the build libraries, and it seems to be working.
However, I did have to use a very specific version of the Selenium driver & support library (and it's not the version that's documented in the Quickstart).(Turns out I had a mix of Selenium library versions on the go. 4.25.0 is fine)