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

Valhalla test updates and disable failing tests #19690

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Valhalla test updates and disable failing tests #19690

merged 5 commits into from
Jun 14, 2024
+289 −322

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Jun 12, 2024 •
edited
Loading

After this change set the Valhalla more tests should be passing. Tests that are not passing are disabled for now.

This change should be merged after #19646

Changes include:

  • removing unused code from ValueTypeTests including the myUnsafe field
  • Replace q signatures with L. Mark all generated value fields as NullRestricted. Previously all value classes were marked as primitive, this most closely matches the behavior.
  • Remove and modify a few tests in ValueTypeTests, disable remaining

The following issues and comments have been created to track the excluded tests:

@theresa-m theresa-m requested a review from hangshao0 June 12, 2024 20:28
@theresa-m theresa-m added comp:vm comp:test project:valhalla Used to track Project Valhalla related work and removed comp:vm comp:test labels Jun 12, 2024
@theresa-m theresa-m force-pushed the vttests_6 branch 2 times, most recently from 2d3e21f to 6168efb Compare June 12, 2024 20:37
@theresa-m theresa-m mentioned this pull request Jun 12, 2024
Closed
@hangshao0
Copy link
Contributor

@theresa-m Please rebase. There is a merge conflict. Also the commit Remove CFR_ACC_VALUE_TYPE and CFR_ACC_PRIMITIVE_VALUE_TYPE is already reviewed and merged in #19631. It should not appear in this PR again.

@theresa-m
Remove unused ValueTypeTests method and myUnsafe
ebe8f16 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Replace q signatures with NullRestricted attribute
ca48415 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Remove tests to match lw5
1cfbd5d 
- testInvalidNestedField is replaced by preload tests
 in ValhallaAttributeTests
- testNoneValueQTypeAsNestedField is replaced by
 nullrestricted tests in ValhallaAttributeTests

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Modify testIdentityObjectOnJLObject to match ri
da2a754 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Copy link
Contributor Author

I have rebased this change.

@theresa-m theresa-m mentioned this pull request Jun 13, 2024
Merged
@theresa-m theresa-m requested a review from hangshao0 June 14, 2024 13:57
@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinuxval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended winval jdknext

@hangshao0
Copy link
Contributor

The Windows build failed due to machine issue:
11:16:56 ERROR: Cannot delete workspace :Unable to delete 'F:\Users\jenkins\workspace\Build_JDKnext_x86-64_windows_valhalla_Personal\openssl\NUL'

Tests that are not passing are disabled for now.

I was assuming all tests should pass. Could you look at the failures in the Linux s390 build ? @theresa-m

@theresa-m
Copy link
Contributor Author

The Windows build failed due to machine issue: 11:16:56 ERROR: Cannot delete workspace :Unable to delete 'F:\Users\jenkins\workspace\Build_JDKnext_x86-64_windows_valhalla_Personal\openssl\NUL'

Tests that are not passing are disabled for now.

I was assuming all tests should pass. Could you look at the failures in the Linux s390 build ? @theresa-m

Hmm yea I'll check it out. I was mostly verifying that on my mac which is not able to run all the variations.

@hangshao0
Copy link
Contributor

The PR build link on Linux s390 is https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/5637/

@theresa-m theresa-m mentioned this pull request Jun 14, 2024
Open
hangshao0
hangshao0 reviewed Jun 14, 2024
View reviewed changes
test/functional/Valhalla/playlist.xml
@@ -63,13 +63,13 @@
<variations>
<variation>-Xjit:count=0</variation>
<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput</variation>
<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
<!--<variation>-Xjit:count=1,disableAsyncCompilation -Xgcpolicy:optthruput -XX:ValueTypeFlatteningThreshold=99999 -XX:-EnableArrayFlattening</variation>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the JIT issue number as a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its there now.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @hzongaro @a7ehuo Some JIT variations of this test are being disabled because of so many failures.

@theresa-m
Disable failing ValueTypeTests
4d9a1a9 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Copy link
Contributor Author

I have disabled the failing tests and updated the issues for excluded tests. Can you try the build again @hangshao0 ?

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinuxval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended alinuxval jdknext

@hangshao0 hangshao0 merged commit 92c330c into eclipse-openj9:master Jun 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangshao0 hangshao0 hangshao0 approved these changes

Assignees
No one assigned
Labels
comp:test project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theresa-m @hangshao0