-
-
Notifications
You must be signed in to change notification settings - Fork 314
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
Add first batch of smoke tests for AdoptOpenJDK #2067
Conversation
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.
Two immediate comments, more review to follow later
IMO, Can ShenandoahTest use Also, do we need to check the JDK version subversion? Is the major JDK version sufficient enough? If so, we can just specify AdoptOpenJDKVendorTest checks very specific things in |
|
||
List<String> command = new ArrayList<>(); | ||
command.add(String.format("%s/bin/java", testJdkHome)); | ||
command.add("-version"); |
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.
Instead of running java -version
, can we just get the info from System.getProperty("java.fullversion")
? If so, we can avoid running the command within java and using BufferedReader to get output.
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.
The purpose of this test is to actually run the command and check its output. We have regressions in this area, currently on ARM.
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.
Issue on ARM is this one: adoptium/temurin-build#2229, does it mean that the property returns correctly but the java -version output does not match the property?
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.
Properties:
java.runtime.name = OpenJDK Runtime Environment
java.runtime.version = 1.8.0_275-b01
java.specification.name = Java Platform API Specification
java.specification.vendor = Oracle Corporation
java.specification.version = 1.8
java.vendor = AdoptOpenJDK
java.vendor.url = https://adoptopenjdk.net/
java.vendor.url.bug = https://github.com/AdoptOpenJDK/openjdk-support/issues
java.version = 1.8.0_275
java.vm.info = mixed mode
java.vm.name = OpenJDK Client VM
java.vm.specification.name = Java Virtual Machine Specification
java.vm.specification.vendor = Oracle Corporation
java.vm.specification.version = 1.8
java.vm.vendor = AdoptOpenJDK
java.vm.version = 25.275-b01
java -version
:
/usr/lib/jvm/jdk8/bin/java -version
openjdk version "1.8.0_275"
OpenJDK Runtime Environment (build 1.8.0_275-b01)
OpenJDK Client VM (build 25.275-b01, mixed mode)
* Tests whether "AdoptOpenJDK" appears as vendor in all the places it is supposed to. | ||
*/ | ||
@Test(groups = {"level.extended"}) | ||
public class AdoptOpenJDKVendorTest { |
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.
As we are building AQA to be a set of tests that can be helpful to all vendors, this test could get updated to be VendorTest (or possibly even better, changed to be a PropertiesTest) that contains a set of test methods that checks the value of a java property is as expected for that vendor.
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.
You want me to define some data source that contains the properties for various vendors so that I can look it up by some key (like the vendor)? Do you want me to do that now?
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.
It's not pretty and we need some value from the outside to tell VendorPropertiesTest
what vendor to expect. A simple map with hard coded values it not going to work except we want to maintain an endless list of values. And the test won't pass with AdoptOpenJDK because we don't set one value correctly.
* Overview</a> | ||
*/ | ||
@Test(groups = {"level.extended"}) | ||
public class ShenandoahTest { |
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.
Are there other features that we might expect to check in the future that would/could also live in a CheckXXOptionAvailableTest?
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.
ZGC, JFR to begin with.
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.
Tests for both added.
|
||
<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd"> | ||
<suite name="AdoptOpenJDK Tests" parallel="none" verbose="2"> | ||
<test name="AdoptOpenJDKTests"> |
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.
As Lan suggested, we can separate the tests into different names:
In hindsight, it would have made some sense to group the ssl/cert test in with CheckXXOptionAvailableTest as a set of FeaturesArePresentTests or something along those lines. If you already have some other tests in mind, can we lay out a plan for other checks that should be added, so we think about how those new tests will be organized alongside the existing ones?
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.
we still can move them together, by moving the contents of functional/security and the contents of functional/adoptopenjdk into functional/packaging, then add this to the testng.xml file
<test name="SecurityTests">
<classes>
<class name="net.adoptopenjdk.test.DefaultTlsFunctionalTest"/>
<class name="net.adoptopenjdk.test.Tls13FunctionalTest"/>
</classes>
</test>
and functional/packaging/playlist.xml contains 3 targets, one for SecurityTests, one for FeaturesArePresentTests and one for CheckPropertiesTests.
Note: often each test target in a playlist contains 100s of test cases. When we structure targets to be all approximately the 'same size / same execution time', it makes for optimal scheduling and parallelization within a CI system.
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.
I'm a bit at a loss here. Is the deciding factor the runtime or the purpose of the tests? The security tests can be run against any OpenJDK and should be successful. The AdoptOpenJDK tests are specific for AdoptOpenJDK. But yeah, it's sometimes hard to decide whether it's a general purpose test or AdoptOpenJDK specific: Shenandoah is not in Oracle builds (it might even be absent from downloads from jdk.java.net), but present in everything built by Red Hat (even 8). In 11.0.9, it requires a compiler flag, but not in 15+. JFR is absent from the upstream binaries we host (at least from some). I don't know whether it requires a flag.
So from my POV it makes sense to have different test groups.
I doubt that it's going to scale. The TLS 1.3 tests check for 8u272, 11 or higher. A JFR test is going to check for 8u262, 11 or higher. Except having countless test targets is desirable.
I'm not sure I'm able to express version/platform combinations with the XML. Furthermore, it would accelerate the sprawling of test targets. |
I think I addressed all concerns and comments that I could. @smlambert It's probably best if you reorganize everything according to your liking. Is it possible to run a single grinder over all JDKs we currently test or is it easier to merge and then adjust afterwards as needed? |
See #2024 for details and motivation.
Grinders: jdk11 j9 xlinux functional / AdoptOpenJDKTests: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5045 - 2/6 tests fail, java.vendor.version contains no numbers AND shenandoah test fails (ShenandoahTest should is not valid against an OpenJ9 impl, can/should to be tagged as such in playlist) jdk8 hs mac functional / AdoptOpenJDKTests: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5046 - pending jdk11 dragonwell xlinux functional / AdoptOpenJDKTests: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5049 - pending |
This is expected. From my POV, the property should contain a version number. It does, for example, in Corretto, but not in AdoptOpenJDK. It only contains "AdoptOpenJDK". |
I see, so that test will fail until someone addresses adoptium/temurin-build#1387 Some observations:
|
I'm curious whether you might be able to figure out a better way. My roadblock was Corretto. It uses version-dependant URLs while AdoptOpenJDK does not.
👍 I wanted to introduce better diagnostics with AssertJ. Its output is so much more helpful by default. |
I think now I have a better understanding of how we will need to scale, divide and conquer these types of tests:
Mixed into this PR are the potential for both types of tests (those that can be reworked to benefit all vendors, and those that may be so specific to AdoptOpenJDK/Adoptium openjdk-build repo that they may live in that repo, and be pulled in and run as part of nightly testing. I will add some additional details and a reworked example of these tests shortly. |
@ShelleyLambert any update on this after your previous posts? It would be good to get something in and available for now that people can contribute to even if it does mean moving things later on |
Will combine this PR, with #2140 and house in a test dir in the openjdk-build repo (to be run as an independent smoke test which must pass in order for the AQA test jobs to be triggered). We will leave this open form until the new combined openjdk-build PR lands. |
I believe we can close this now that these tests were delivered as part of adoptium/temurin-build#2397. I will leave #2024 and #2258 open until we have added the release file tests and incorporated the test job in as part of the build pipeline. |
The tests check whether Shenandoah GC is enabled on the platforms it should be enabled on and also whether AdoptOpenJDK pops up in the
java -version
output and some related properties. It's currently less about an comprehensive test coverage and more about working out details like version and platform detection that others can build upon.JdkPlatform
andJdkVersion
exist because I haven't figured out a way to instruct TKG on a test by test basis to run or skip a test. If there's a better way to do things, please let me know!