Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Add first batch of smoke tests for AdoptOpenJDK #2067

wants to merge 1 commit into from

Conversation

aahlenst
Copy link
Contributor

@aahlenst aahlenst commented Nov 21, 2020

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 and JdkVersion 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!

@karianna karianna added this to the November 2020 milestone Nov 24, 2020
@aahlenst aahlenst changed the title Check whether Shenandoah GC is enabled in Adopt Add first batch of smoke tests for AdoptOpenJDK Nov 26, 2020
@aahlenst aahlenst marked this pull request as ready for review November 26, 2020 13:02
Copy link
Contributor

@karianna karianna left a 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

functional/adoptopenjdk/build.xml Outdated Show resolved Hide resolved
functional/adoptopenjdk/build.xml Show resolved Hide resolved
@smlambert smlambert requested a review from llxia November 27, 2020 17:08
@llxia
Copy link
Contributor

llxia commented Nov 27, 2020

IMO, ShenandoahTest and AdoptOpenJDKVendorTest are very different. AdoptOpenJDKVendorTest is general for all AdoptOpenJDK SDK and all versions. And ShenandoahTest is specific to version 11.0.9+. They should be in the separated test targets in palylist.xml.

Can ShenandoahTest use <platformRequirements> to limit on what platform it should be run on? For example, https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/perf/idle_micro/playlist.xml#L56

Also, do we need to check the JDK version subversion? Is the major JDK version sufficient enough? If so, we can just specify <subset>11+</subset>

AdoptOpenJDKVendorTest checks very specific things in java -version for SDK that is generated from AdoptOpenJDK. This will fail for SDK that is generated from other places (i.e., Openj9, IBM, etc). With Add support for JDK_VENDOR in playlists, it will help this issue. But for now, I think we can limit it using impl. For example, <impl>hotspot</impl>

functional/adoptopenjdk/build.xml Outdated Show resolved Hide resolved

List<String> command = new ArrayList<>();
command.add(String.format("%s/bin/java", testJdkHome));
command.add("-version");
Copy link
Contributor

@llxia llxia Nov 27, 2020

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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">
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@aahlenst
Copy link
Contributor Author

And ShenandoahTest is specific to version 11.0.9+. They should be in the separated test targets in palylist.xml.

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.

Can ShenandoahTest use to limit on what platform it should be run on?

I'm not sure I'm able to express version/platform combinations with the XML. Furthermore, it would accelerate the sprawling of test targets.

@aahlenst
Copy link
Contributor Author

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?

@smlambert
Copy link
Contributor

smlambert commented Nov 30, 2020

Grinders:
jdk11 hs xlinux functional / AdoptOpenJDKTests: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/5044 - 1/6 tests fail, java.vendor.version contains no numbers

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

@aahlenst
Copy link
Contributor Author

java.vendor.version contains no numbers

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".

@smlambert
Copy link
Contributor

I see, so that test will fail until someone addresses adoptium/temurin-build#1387

Some observations:

  • likely we will reorg the AQAtic code to TKG or somewhere where it can be shared/available with more than just functional test material (so system/perf/external/openjdk test groups could take advantage of those utility classes / not required as part of this PR)
  • think there is a cleaner way to handle multi-vendor... we can use testng parameterization to have a generalized test that runs with different parameters depending on the auto-detect and or params passed to test pipelines via upstream trigger. Trying it out locally by cloning your branch and making some updates (similar to example in this tutorial) (and to resolve Add support for JDK_VENDOR in playlists  TKG#115)
  • want to also print out 'value' of properties on each assert statement in each test method (not just state "contains no numbers", but I immediately want to know what 'value' is.

@aahlenst
Copy link
Contributor Author

aahlenst commented Dec 1, 2020

think there is a cleaner way to handle multi-vendor... we can use testng parameterization to have a generalized test that runs with different parameters depending on the auto-detect and or params passed to test pipelines via upstream trigger.

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.

want to also print out 'value' of properties on each assert statement in each test method (not just state "contains no numbers", but I immediately want to know what 'value' is.

👍 I wanted to introduce better diagnostics with AssertJ. Its output is so much more helpful by default.

@smlambert
Copy link
Contributor

I think now I have a better understanding of how we will need to scale, divide and conquer these types of tests:

  • clarify which tests would be beneficial to ALL or most vendors (these types of tests should stay in the openjdk-tests repo and form part of AQA which is of value to many)
  • tests that are truly only ever going to be specific to one vendor can live elsewhere and be incorporated via VENDOR_REPO/VENDOR_SHA/VENDOR_BRANCH mechanism we have

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.

@sxa
Copy link
Member

sxa commented Dec 23, 2020

@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

@smlambert
Copy link
Contributor

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.

@smlambert
Copy link
Contributor

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.

@smlambert smlambert closed this Feb 11, 2021
@karianna karianna added this to the February 2021 milestone Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants