-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle older versions of Gradle #149
Conversation
Thank you for this work! Though the CI takes longer but it help us find a lot of regressions so it's worth doing. Just curious how are those Gradle versions picked? Is there any rule that we can follow in the future? |
It was a bit painful - I tried a few random versions at first and then tests started failing so I had to read through release notes to find what version caused the API change and kept that version in the test list. The release notes are all here I think we should just keep the latest version in the list up to date. Currently it's 8.8, next we should change it to 8.9. Whenever the tests fail on a new version because of API change then we should keep that version in the list forever. |
This makes sense! BTW, do you think it worth using the Gradle TestKit for the cross version test purpose? I haven't used it before but looking at the document looks like it's a recommended way to do so? |
BTW, this is just an ask, if the testkit is fit for our scenario, it's not necessary to do so in this PR. We can file it as a engineering debt and address it in the future. |
@jdneo I'm not sure. It may be easier to debug - at the moment I just add the jvm options and attach to the process |
Tracked by #151. |
plugin/src/test/java/com/microsoft/java/bs/gradle/plugin/GradleBuildServerPluginTest.java
Show resolved
Hide resolved
da34bf4
to
6580779
Compare
6580779
to
013a0b4
Compare
import org.gradle.api.file.SourceDirectorySet; | ||
import org.gradle.api.internal.tasks.scala.MinimalScalaCompileOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @Arthurm1 |
Co-authored-by: Arthur McGibbon <arthur.mcgibbon@h3im.com>
init named pipe test build Server named pipe connection update named pipe stream solution finish forward message finish merge and named pipe fix - Handle older versions of Gradle (microsoft#149) Co-authored-by: Arthur McGibbon <arthur.mcgibbon@h3im.com> fix - Add JDK 22 compatibility support (microsoft#152) Signed-off-by: Sheng Chen <sheche@microsoft.com> fix - Return the root cause of the message (microsoft#156) feat - Add support for running tests (microsoft#144) Co-authored-by: Arthur McGibbon <arthur.mcgibbon@h3im.com> Co-authored-by: Sheng Chen <sheche@microsoft.com> add Launcher by namedPipe with backwards compatible
I had some issues with older versions of Gradle not working properly so I've changed each test in
GradleBuildServerPluginTest
to take gradle version as a parameter and run the test with that version.I may have gone overboard in number of versions that are tested. They're all the ones I could see that the various checks in the code test for. The downside is that the plugin tests take a lot longer although it's good to know all versions work.
Adding these tests showed up a number of things that don't work in older versions that I've fixed in this PR...
internal
classes.What is the earliest version that should be supported?