Parse test target for jitaas flag

[AdamBrousseau]

• Adds ability to pass a TESTS_TARGETS with '+jitaas' in the name.
• Sets flag TEST_FLAG=JITAAS and passes to Test job

Issue #2893
Related adoptium/aqa-tests#584, #3059
[skip ci]
Signed-off-by: Adam Brousseau [email protected]

[AdamBrousseau]

This is what we have come up with for a proposed way to handle turning on jitaas testing via either a PR or regular build.
In a regular build, you would add .jitaas to any test target you wanted to run. In a PR build you would basically do the same. For example Jenkins test sanity.functional.jitaas xlinux jdk11. We still call down to the same sanity.functional job but with the parameter TEST_FLAG=JITAAS. This allows you to run both jitaas tests and non jitaas tests in the same PR command. Ex Jenkins test sanity.functional,sanity.functional.jitaas xlinux jdk11

Please note: This will not work via PRs until #2728 is resolved
cc @smlambert @renfeiw @jdekonin @DanHeidinga @pshipton @harryyu1994

[smlambert]

As discussed with @AdamBrousseau the caveat of this approach is that committers/users may be confused and believe that sanity.functional.jitaas is an actual test target that they could run, which it is most definitely not.

This approach does overcome the barrier of how to format and retrieve input from a git comment to trigger PR builds, and given our short window to enable this testing, we should likely proceed with it.

I do not want to see us continue to overload the test targets via the PR build plugin everytime a new feature request comes in. It will be good for us to consciously keep the PR inputs aligned with the actual manual/commandline input for testing, otherwise make it a maintenance/doc nightmare to explain the mappings introduced over time. We see that in the internal/proprietary system we are still pulling ourselves out of, there are far too many cases of "we mapped this to this to this to this..." to the point where we no longer even understand how that evolution/hackery ever occurred.

[AdamBrousseau]

The alternative would be something like Jenkins test sanity.functional xlinux jdk11 TEST_FLAG=JITAAS depends ....
Perhaps we should update the depends syntax to be DEPENDS=eclipse/omr#1,ibmruntimes/openj9-openjdk-jdk11#2 since the code currently takes everything after depends as a space separated list of dependencies. You could probably argue that PLATFORMS, VERSIONS and TESTS_TARGETS should be used this way as well. That way order no longer matters.

[pshipton]

A slight modification would be sanity.functional+jitaas which maybe keeps the parsing easy for faster delivery, but is more explainable, less confusing, to users trying to figure out the test target.

And then agree on a longer term strategy such as suggested in #3101 (comment), although I'm not a fan of having to specify PLATFORMS=, VERSIONS=, etc.

[smlambert]

sanity.functional+jitaas
that is a good compromise... ship it!

[AdamBrousseau]

Updated change. Please review & merge.

[smlambert]

Jenkins test sanity+jitaas xlinux jdk11

[AdamBrousseau]

That syntax would work if this and #2836 was merged ;)

[harryyu1994]

sorry forgot to ask this earlier, has the JVM EXTRA_OPTION "-XX:JITaaSClient " already been dealt with as well?

[AdamBrousseau]

Good question. I don't see where it would have been added yet. I thought it would have been in #3059 or adoptium/aqa-tests#584 but it isn't there. I do however see this. So I may have to make another change to also pass EXTRA_OPTIONS or JVM_OPTIONS to the test jobs. @smlambert or @renfeiw please confirm.

[smlambert]

I don't think this was clearly communicated in the initial request (and I have not seen a reply to my question, how are customers expected to invoke this feature)...

But anyway, there number of ways to do this. Testing with additional cmdline options, can be done by introducing new variants in playlist.xml file. I sense the expectation here is that you assume we will apply this commandline option across the board to all tests (that all tests should be able to be run with jitaas).

If so, then we can leverage EXTRA_OPTIONS env var and set the
export EXTRA_OPTIONS="-XX:JITaaSClient"

(JVM_OPTIONS replaces existing options which is not desirable in this case, EXTRA_OPTIONS appends additional commandline options).

[smlambert]

@renfeiw - when we setup jitaas server, can we also append -XX:JITaaSClient ?

[AdamBrousseau]

Ie. Set EXTRA_OPTIONS here

[smlambert]

@AdamBrousseau - thanks. We know where to set it, I was asking if @renfeiw can prep a PR. You do not need to help chase down test solutions, I know your plate is full in releng/build space.

[renfeiw]

I can add EXTRA_OPTIONS=-XX:JITaaSClient in the testEnv.mk, but my concern is this hides the EXTRA_OPTIONS away. It will break if a user explicitly exports another EXTRA_OPTIONS and forget to append -XX:JITaaSClient in the export. My understanding is, when a user wants to locally run the tests with jitaas, he/she has to provide TEST_FLAG and EXTRA_OPTIONS manually. In the build environment, the EXTRA_OPTIONS can be set along with TEST_FLAG when +jitaas exists. Another way is to introduce a second option JITAAS_OPTIONS, and define it in the testEnv.mk.

[smlambert]

yes, we can proceed with the separate JITAAS_OPTIONS approach.

[AdamBrousseau]

Can we not do something like EXTRA_OPTIONS+=... in the makefile?

[renfeiw]

Yes, we can. It's the same idea with the approach JITAAS_OPTIONS, instead of directly appending it in the mkgen.pl, we append it in the EXTRA_OPTIONS. I guess we need to use override EXTRA_OPTIONS+=..., as otherwise the cmdline argument will override this makefile assignment. I would prefer the JITAAS_OPTIONS, as EXTRA_OPTIONS was designed for user's extra option only.

[AdamBrousseau]

I guess my concern is from the build side, we're already passing TEST_FLAG=JITAAS, I'd rather not pass another, somewhat redundant, option.

[renfeiw]

You don't need to pass another variable. I will define JITAAS_OPTIONS in the testEnv.mk, and use it in the mkgen.pl.