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.

Sign up for GitHub

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

Jump to bottom

OJDK MHs test excludes + Process a list of test exclude files #12779

Merged
merged 3 commits into from
Jun 4, 2021
Merged

OJDK MHs test excludes + Process a list of test exclude files #12779

merged 3 commits into from
Jun 4, 2021

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented May 27, 2021 •
edited
Loading

Commit 1: Process a list of test exclude files

Currently, IncludeExcludeTestAnnotationTransformer only processes a single
exclude file.

It has been updated to process a list of exclude files.

The list can be provided via the EXCLUDE_FILE environment variable where the
exclude files are separated by commas.

The name of the environment variable is left unchanged. So, the existing
scripts do not need to be updated.

Commit 2: Create a test exclude file for OJDK MHs

Commit 3: Add a system property to check if OJDK MHs are enabled

The new system property can be checked through -XshowSettings:properties.

TODO: #12811

Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

@babsingh babsingh force-pushed the exclude_ojdkmh_tests branch 2 times, most recently from 1845973 to be569b9 Compare May 27, 2021 01:42
@babsingh babsingh added comp:test project:MH Used to track Method Handles related work labels May 27, 2021
@babsingh
Copy link
Contributor Author

babsingh commented May 27, 2021 •
edited
Loading

This PR is created based upon the discussion in #11935.

I created adoptium/TKG#198 to append OJDK MH test exclude file to EXCLUDE_FILE. I didn't find a way to do a runtime check for OJDK MHs. Does anyone have suggestions for a OJDK MH runtime check?

How should I test these changes? This PR will require adoptium/TKG#198. In OpenJ9 personal builds, I didn't find a field to specify a custom TKG repo.

Also, any feedback is welcome on the approach taken in this PR.

Note: Use hide whitespace changes to view the code changes.

++ @llxia @renfeiw @smlambert @pshipton

@pshipton
Copy link
Member

I didn't find a way to do a runtime check for OJDK MHs.

Would it work to temporarily add a system property when the java preprocessor tag OPENJDK_METHODHANDLES is enabled? The system properties are visible via -XshowSettings:properties

@babsingh
Copy link
Contributor Author

babsingh commented May 27, 2021 •
edited
Loading

Would it work to temporarily add a system property when the java preprocessor tag OPENJDK_METHODHANDLES is enabled?

Yes, it should work. The system property can be added in vmprops.c:initializeSystemProperties during startup using the native OJDK MH flag. In TKG/settings.mk, we can check for the system property in $TEST_JDK_HOME.

@babsingh babsingh force-pushed the exclude_ojdkmh_tests branch 2 times, most recently from 7cfaa98 to 47d9e49 Compare June 1, 2021 15:24
@babsingh babsingh marked this pull request as ready for review June 1, 2021 20:12
@babsingh babsingh mentioned this pull request Jun 1, 2021
Merged
@babsingh
Copy link
Contributor Author

babsingh commented Jun 1, 2021 •
edited
Loading

@pshipton @llxia @smlambert This PR is required for #12779 adoptium/TKG#198. I have tested these changes locally. They work correctly with OJ9 MHs and OJDK MHs. I will be on leave starting next Monday. Can we have this PR merged by Friday?

@babsingh
Copy link
Contributor Author

babsingh commented Jun 1, 2021

It will be easier to code review after applying: Hide whitespace changes.

@pshipton
Copy link
Member

pshipton commented Jun 1, 2021

This PR is required for #12779

This PR is #12779, I assume you mean adoptium/TKG#198

@babsingh
Copy link
Contributor Author

babsingh commented Jun 1, 2021

This PR is #12779, I assume you mean adoptium/TKG#198

Correct. I have amended my previous comment.

@babsingh babsingh mentioned this pull request Jun 1, 2021
Open
@llxia
Copy link
Contributor

llxia commented Jun 2, 2021

Tested using this PR and adoptium/TKG#198 + JDK8 nightly:
https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/47/

15:57:45  [IncludeExcludeTestAnnotationTransformer] [INFO] EXCLUDE_FILE environment variable: /home/jenkins/workspace/Grinder/openjdk-tests/TKG/../TestConfig/resources/excludes/latest_exclude_8.txt
15:57:45  [IncludeExcludeTestAnnotationTransformer] [INFO] Processing exclude file: /home/jenkins/workspace/Grinder/openjdk-tests/TKG/../TestConfig/resources/excludes/latest_exclude_8.txt
15:57:45  ...

llxia
llxia approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@babsingh
Process a list of test exclude files
9337026 
Currently, IncludeExcludeTestAnnotationTransformer only processes a single
exclude file.

It has been updated to process a list of exclude files.

The list can be provided via the "EXCLUDE_FILE" environment variable where the
exclude files are separated by commas.

The name of the environment variable is left unchanged. So, the existing
scripts do not need to be updated.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Create a test exclude file for OJDK MHs
180f4f4 
Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Add a system property to check if OJDK MHs are enabled
2d75afd 
The new system property can be checked through "-XshowSettings:properties".

TODO: eclipse-openj9#12811

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Copy link
Contributor Author

babsingh commented Jun 3, 2021

I have updated commit 2: "Create a test exclude file for OJDK MHs". @EricYangIBM has fixed #11528 via #12799 and #12806. So, #11528 related failures no longer need to be excluded.

@babsingh
Copy link
Contributor Author

babsingh commented Jun 3, 2021 •
edited
Loading

@fengxue-IS @JasonFengJ9 I have disabled the tests related to the following issues: #11924, #11927 and #12690. I have tagged you since you are either working or will work to fix these issues. Once a fix is merged, the excluded test(s) should be re-enabled.

fyi @tajila

smlambert
smlambert approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this, think there is potential follow-up work as noted.

test/Utils/src/org/openj9/test/util/IncludeExcludeTestAnnotationTransformer.java Show resolved Hide resolved
@tajila
Copy link
Contributor

tajila commented Jun 4, 2021

jenkins test sanity xlinuxojdk292 jdk16 depends ibmruntimes/openj9-openjdk-jdk16#29

@tajila
Copy link
Contributor

tajila commented Jun 4, 2021

Jenkins test sanity plinux jdk16

@tajila tajila mentioned this pull request Jun 4, 2021
Open
@tajila
Copy link
Contributor

tajila commented Jun 4, 2021

The PR build I launched won't pass without adoptium/TKG#198. @babsingh has verified that the changes work with the TKG PR so I will merge this now.

@tajila tajila merged commit f6bdf7a into eclipse-openj9:master Jun 4, 2021
@pshipton pshipton mentioned this pull request Jun 10, 2021
Merged
@fengxue-IS fengxue-IS mentioned this pull request Jun 15, 2021
Merged
@babsingh babsingh deleted the exclude_ojdkmh_tests branch February 18, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlambert smlambert smlambert approved these changes

@pshipton pshipton pshipton approved these changes

@llxia llxia llxia approved these changes

Assignees
No one assigned
Labels
comp:test project:MH Used to track Method Handles related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@babsingh @pshipton @llxia @tajila @smlambert