Support multiple required in features tag #614

annaibm · 2024-09-20T15:58:05Z

Removed null statement from required feature check to ensure all required features are processed.
Allows the code to continue checking all feature flags, even after finding a required flag in testFlags.

related:https://github.ibm.com/runtimes/backlog/issues/1525

llxia · 2024-09-20T18:38:58Z

src/org/testKitGen/TestInfoParser.java

-				if (!isFeatureInTestFlags(testFlags, entry.getKey())) {
-					return null;
+				if (isFeatureInTestFlags(testFlags, entry.getKey())) {
+					break;


With this change, if there is no test flag and feature match, the test will run. This is not expected behavior.
Grinder - test run with TEST_FLAG=empty.

annaibm · 2024-09-23T14:19:01Z

Grinder:
TEST_FLAG: FIPS140_3_OpenJCEPlusFIPS
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43441/rebuild/parameterized
TEST_FLAG: FIPS140_3_OpenJCEPlusFIPS.FIPS140-3
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43440/
TEST_FLAG: EMPTY
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43439/

llxia · 2024-09-23T15:44:08Z

feature required is an optional value. It may or may not exist. We cannot return null if requiredFeatureFound is not found. https://github.com/adoptium/TKG/pull/614/files#diff-1ce05e4d3d428c3c8430b54f2905415f30fd13919ff8269e99c127485d69093eR111-R113 is not correct.

For example: https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43462/

llxia · 2024-09-23T15:49:34Z

This existing logic in TKG is not straightforward. To make this simple, I think we should make assumption that we cannot use feature required with other featureOpts (otherwise, we may need to rewrite the entire logic). For example, FIPS140_2:nonapplicable and FIPS140_3_OpenJCEPlusFIPS:required cannot be used together. If they are used together, only required will be considered.

With this assumption, the logic can be updated to


 for (Map.Entry<String,String> entry : ti.getFeatures().entrySet()) {
			String featureOpt = entry.getValue().toLowerCase();
			if (featureOpt.equals("required")) {
				 // create required feature list - ListA
			} else if (featureOpt.equals("nonapplicable")) {
				// Do not generate make target if the test is not applicable for one feature defined in TEST_FLAG
				if (isFeatureInTestFlags(testFlags, entry.getKey())) {
					return null;
				}
			} else if (featureOpt.equals("applicable") || featureOpt.equals("explicit")) {
				// Do nothing
			} else {
				System.err.println("Error: Please provide a valid feature parameter in test " + ti.getTestCaseName() + ". The valid string is <feature_name>:[required|applicable|nonapplicable|explicit].");
				System.exit(1);
			}
		}


// Assume that we cannot use feature required with other featureOpts
If ListA is not empty
    if testFlag is not in ListA
          return null

@LongyuZhang what do you think?

annaibm · 2024-09-23T16:12:50Z

<features>
    <feature>FIPS140_2:nonapplicable</feature>
    <feature>FIPS140_3_OpenJCEPlusFIPS:required</feature>
    <feature>FIPS140_3_OpenJCEPlusFIPS.FIPS140-3:required</feature>
</features>

As I understand ,I think the the issue is when we are having 2 required field in features tag and it’s expecting the test flags to have both the flags mentioned but we just give one flag for eg:FIPS140_3_OpenJCEPlusFIPS or FIPS140_3_OpenJCEPlusFIPS.(which can be valid or not vice versa depends on order of iteration picked).
But the presence of the other feature (FIPS140_3_OpenJCEPlusFIPS.FIPS140-3 or FIPS140_3_OpenJCEPlusFIPS ) in the list is the reason for the error, as it is expected in the test flags but not found there. This is causing the return null in your code, preventing the generation of the required tests and leading to the error message.

annaibm · 2024-09-23T16:52:15Z

Grinder links :
jdk_other
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43482/

TEST_FLAG: EMPTY
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43486/

FIPS140_3_OpenJCEPlusFIPS.FIPS140-3

https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43488/

FIPS140_3_OpenJCEPlusFIPS
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43487/

llxia · 2024-09-23T17:17:33Z

I assume that we cannot use feature required with other featureOpts. This is true with your current version. If required flag is matched, ignore other featureOpts.

llxia · 2024-09-23T17:56:51Z

Discussed with @LongyuZhang , before this change, we need to update the existing playlist.xml. Otherwise, it will break existing testing.

https://github.com/eclipse-openj9/openj9/blob/16ab797349b9bda5a5de2a9ff27ef2d932a4716a/test/functional/cmdLineTests/criu/playlist.xml#L246 - remove <feature>CRIU:required</feature>

Also, we should update the doc to clearly state the assumption:

or relationship is expected in <features>
required should not be used with other feature options

annaibm · 2024-09-23T18:43:21Z

@llxia Please find the draft PR: eclipse-openj9/openj9#20215 to remove CRIU featureOption.
- remove <feature>CRIU:required</feature>

llxia · 2024-09-24T15:27:29Z

Please run TKG PR test.

annaibm · 2024-09-24T15:28:57Z

Grinder links:

TARGET:jdk_other
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43526/
TEST_FLAG: EMPTY
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43483/

TEST_FLAG:FIPS140_3_OpenJCEPlusFIPS.FIPS140-3
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43527/

TEST_FLAG:FIPS140_3_OpenJCEPlusFIPS
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43528/

TEST_FLAG:FIPS140_2
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43530/

llxia · 2024-09-24T15:54:45Z

Can we update the title to reflect the change? Update feature flag handling for required features -> Support multiple required in features tag

annaibm · 2024-09-24T18:25:14Z

run tkg-test

github-actions · 2024-09-24T18:25:25Z

@annaibm TKG test build started, workflow Run ID: 11019710616

github-actions · 2024-09-24T18:28:27Z

@annaibm Build(s) failed, workflow Run ID: 11019710616

annaibm · 2024-09-25T13:58:53Z

run tkg-test

github-actions · 2024-09-25T13:59:08Z

@annaibm TKG test build started, workflow Run ID: 11034601010

github-actions · 2024-09-25T14:02:17Z

@annaibm Build(s) successful, workflow Run ID: 11034601010

llxia · 2024-09-25T14:43:02Z

scripts/testTKG/test_platformRequirements.py


 	if 'test_arch_390_z15_0' not in passed:
 		skipped.add('test_arch_390_z15_0')
-
+	


Please remove extra space

- Removed `null` statement from `required` feature check to ensure all required features are processed. - Allows the code to continue checking all feature flags, even after finding a required flag in `testFlags`. - Added test_not_arch_390_z15plus_0 into passed list for running TKG test PR in github actions as no microarch version is detected. related:https://github.ibm.com/runtimes/backlog/issues/1525 Signed-off-by: Anna Babu Palathingal <[email protected]>

llxia · 2024-09-25T15:35:38Z

Thanks @annaibm. Since TKG serves as the core framework layer, it could significantly impact testing behavior. For due diligence, could you please also run sanity.functional? Say, using JDK11 on xlinux. Compare with nightly runs and the TAP file info (TEST TARGETS RESULTS SUMMARY: TOTAL: xxx EXECUTED: xxx PASSED: xxx FAILED: xxx DISABLED: xxx SKIPPED: xxx) should be the same.

annaibm · 2024-09-26T13:26:03Z

Grinder link:
TARGET: sanity.functional
https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43590/
TEST TARGETS SUMMARY:

TOTAL : 431
EXECUTED : 385
PASSED : 385
FAILED : 0
DISABLED : 8
SKIPPED : 38

Comparison with nightly -> https://hyc-runtimes-jenkins.swg-devops.com/job/Test_openjdk11_j9_sanity.functional_x86-64_linux/931/tapTestReport/
TEST TARGETS SUMMARY:

TOTAL : 98
EXECUTED : 98
PASSED : 98
FAILED : 0
DISABLED : 0
SKIPPED : 0

TEST TARGETS SUMMARY:

TOTAL : 23
EXECUTED : 23
PASSED : 23
FAILED : 0
DISABLED : 0
SKIPPED : 0

TEST TARGETS SUMMARY:

TOTAL : 310
EXECUTED : 264
PASSED : 264
FAILED : 0
DISABLED : 8
SKIPPED : 38

llxia

Thanks @annaibm

LongyuZhang

LGTM

annaibm mentioned this pull request Sep 20, 2024

Limit OpenJcePlusTests to run with FIPS140-3 profile only adoptium/aqa-tests#5638

Merged

llxia reviewed Sep 20, 2024

View reviewed changes

annaibm force-pushed the testflag branch from 21e1cb7 to 3a1a75e Compare September 23, 2024 13:46

annaibm requested a review from llxia September 23, 2024 14:19

llxia mentioned this pull request Sep 23, 2024

Remove CRIU required in cmdLineTester_criu_jitserverAcrossCheckpoint eclipse-openj9/openj9#20215

Merged

annaibm marked this pull request as ready for review September 24, 2024 13:19

annaibm force-pushed the testflag branch from 824424b to 41a8c20 Compare September 24, 2024 13:21

annaibm changed the title ~~Update feature flag handling for required features~~ Support multiple required in features tag Sep 24, 2024

annaibm force-pushed the testflag branch 2 times, most recently from 278b548 to bae4614 Compare September 25, 2024 13:57

llxia reviewed Sep 25, 2024

View reviewed changes

annaibm force-pushed the testflag branch from bae4614 to 8733901 Compare September 25, 2024 14:47

annaibm requested a review from llxia September 25, 2024 14:48

llxia approved these changes Sep 26, 2024

View reviewed changes

llxia requested a review from LongyuZhang September 26, 2024 17:06

LongyuZhang approved these changes Sep 26, 2024

View reviewed changes

LongyuZhang merged commit d28073b into adoptium:master Sep 26, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple required in features tag #614

Support multiple required in features tag #614

annaibm commented Sep 20, 2024

llxia Sep 20, 2024

annaibm commented Sep 23, 2024

llxia commented Sep 23, 2024

llxia commented Sep 23, 2024

annaibm commented Sep 23, 2024 •

edited

Loading

annaibm commented Sep 23, 2024

llxia commented Sep 23, 2024

llxia commented Sep 23, 2024

annaibm commented Sep 23, 2024

llxia commented Sep 24, 2024

annaibm commented Sep 24, 2024

llxia commented Sep 24, 2024

annaibm commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

annaibm commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

llxia Sep 25, 2024

llxia commented Sep 25, 2024

annaibm commented Sep 26, 2024

llxia left a comment

LongyuZhang left a comment


		if 'test_arch_390_z15_0' not in passed:
		skipped.add('test_arch_390_z15_0')

Support multiple required in features tag #614

Support multiple required in features tag #614

Conversation

annaibm commented Sep 20, 2024

llxia Sep 20, 2024

Choose a reason for hiding this comment

annaibm commented Sep 23, 2024

llxia commented Sep 23, 2024

llxia commented Sep 23, 2024

annaibm commented Sep 23, 2024 • edited Loading

annaibm commented Sep 23, 2024

llxia commented Sep 23, 2024

llxia commented Sep 23, 2024

annaibm commented Sep 23, 2024

llxia commented Sep 24, 2024

annaibm commented Sep 24, 2024

llxia commented Sep 24, 2024

annaibm commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

annaibm commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

llxia Sep 25, 2024

Choose a reason for hiding this comment

llxia commented Sep 25, 2024

annaibm commented Sep 26, 2024

llxia left a comment

Choose a reason for hiding this comment

LongyuZhang left a comment

Choose a reason for hiding this comment

annaibm commented Sep 23, 2024 •

edited

Loading