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

Update NullRestrictedTypeOptTests to build with Valhalla lw5 #18275

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Update NullRestrictedTypeOptTests to build with Valhalla lw5 #18275

merged 2 commits into from
Oct 19, 2023

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Oct 12, 2023 •
edited
Loading

  • Split Valhalla tests into qtypes and lw5 branch
  • Update NullRestrictedTypeOptTests to compile with lw5

Related to #18157

@theresa-m theresa-m added project:valhalla Used to track Project Valhalla related work comp:test labels Oct 12, 2023
@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch from aad7747 to b95db7a Compare October 12, 2023 15:52
@hangshao0 hangshao0 requested a review from hzongaro October 12, 2023 18:20
@hangshao0
Copy link
Contributor

@hzongaro I added you as reviewer as this test was originally created by you.
FYI @a7ehuo

@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch 2 times, most recently from c61e6b1 to 7590d45 Compare October 13, 2023 14:05
@hangshao0 hangshao0 self-assigned this Oct 13, 2023
hangshao0
hangshao0 reviewed Oct 13, 2023
View reviewed changes
test/functional/Valhalla/testng.xml Outdated
@@ -47,7 +47,7 @@
</test>
<test name="NullRestrictedTypeOptTests">
<classes>
<class name="org.openj9.test.lworld.NullRestrictedTypeOptTests"/>
<!--<class name="org.openj9.test.lworld.NullRestrictedTypeOptTests"/>-->
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there will be more tests to be temporarily disabled ? If yes, it might be worth creating an issue to track these disabled tests and include the issue number here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can use the existing issue #18157 to list and track the disabled tests.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, should we keep the old versions of these tests alive while keywords like primitive and the Q signature prefix still work?

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 think both makes sense, we'll just have to be mindful of making sure any new tests get reflected in each set.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the old versions of these tests alive while keywords like primitive and the Q signature prefix still work?

Looks safer to me that we keep the old version alive.

Copy link
Contributor Author

@theresa-m theresa-m Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay I'll modify my pr to keep both.

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 changed it so there is one version of NullRestrictedTypeOptTests in src_lw5 and one in src_qtypes with the idea that you could enable one or the other as needed. Of course src_lw5 doesn't work completely yet since there are many other tests that need updating. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I think your change this looks good.

hzongaro
hzongaro reviewed Oct 16, 2023
View reviewed changes
test/functional/Valhalla/src_lw5/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated
@@ -52,7 +54,7 @@ public EscapeException(Object o) {
}

public static int result = 0;
public static PrimPair[] parr = new PrimPair[] { new PrimPair(3, 4), new PrimPair(0, 0), new PrimPair(0, 0) };
public static PrimPair![] parr = new PrimPair[] { new PrimPair(3, 4), new PrimPair(0, 0), new PrimPair(0, 0) };
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that the right-hand side of the initialization should allocate a null-restricted array:

public static PrimPair![] parr = new PrimPair![] { new PrimPair(3, 4), new PrimPair(0, 0), new PrimPair(0, 0) };

Copy link
Contributor Author

@theresa-m theresa-m Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks you're right. I've updated the three you pointed out.

public static PrimPair![] parr = new PrimPair![] { new PrimPair(3, 4), new PrimPair(0, 0), new PrimPair(0, 0) };

gave an assertion error in the latest lw5 compiler so I declared it for now as

public static FlattenablePair![] parr = new FlattenablePair![3];
static {
/* This syntax is a workaround since initializing the values of a NullRestricted array during 
	* its declaration throws an assertion error in the lw5 compiler. */
parr[0] = new FlattenablePair(3, 4);
parr[1] = new FlattenablePair(0, 0);
parr[2] = new FlattenablePair(0, 0);
}

test/functional/Valhalla/src_lw5/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated Show resolved Hide resolved
test/functional/Valhalla/src_lw5/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated Show resolved Hide resolved
test/functional/Valhalla/testng.xml Outdated
@@ -47,7 +47,7 @@
</test>
<test name="NullRestrictedTypeOptTests">
<classes>
<class name="org.openj9.test.lworld.NullRestrictedTypeOptTests"/>
<!--<class name="org.openj9.test.lworld.NullRestrictedTypeOptTests"/>-->
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, should we keep the old versions of these tests alive while keywords like primitive and the Q signature prefix still work?

@theresa-m theresa-m mentioned this pull request Oct 16, 2023
Open
@theresa-m
Split Valhalla tests into qtypes and lw5 branch
e803bf5 
The qtypes tests will be run by default. The lw5 tests are a work in progress to compile tests with the latest Valhalla compiler and eventually will replace the qtypes tests once qtypes are removed.

Signed-off-by: Theresa Mammarella <[email protected]>
@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch 2 times, most recently from 019f586 to 0d74c9a Compare October 17, 2023 15:19
hangshao0
hangshao0 approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM.

hzongaro
hzongaro reviewed Oct 18, 2023
View reviewed changes
test/functional/Valhalla/src_lw5/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated
}

// An array whose component type is a null-restricted value class
public static NestedFlattenablePair![] nestedflattenablearr = new NestedFlattenablePair[] { new NestedFlattenablePair(11, 12, 13, 14) };
Copy link
Member

Choose a reason for hiding this comment

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

Sorry - I missed this one the first time around. I think the right-hand side of this one needs to allocate a null-restricted array as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops thanks for that. I've updated this array.

@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch from 0d74c9a to 5e80e8d Compare October 18, 2023 17:30
hzongaro
hzongaro approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@hangshao0
Copy link
Contributor

There is a line ending check failure ? @theresa-m

@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch from 5e80e8d to c7f521f Compare October 18, 2023 20:44
@theresa-m
Copy link
Contributor Author

The line ending check is passing now.

@hangshao0
Copy link
Contributor

Jenkins test sanity aixval jdknext

@theresa-m theresa-m force-pushed the lw5_nullrestricted_tests branch from c7f521f to 51d549a Compare October 19, 2023 14:04
@hangshao0
Copy link
Contributor

Jenkins test sanity aixval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended winval jdknext

@hangshao0 hangshao0 merged commit dc28d58 into eclipse-openj9:master Oct 19, 2023
@theresa-m theresa-m mentioned this pull request Oct 20, 2023
Merged
@theresa-m theresa-m deleted the lw5_nullrestricted_tests branch December 6, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzongaro hzongaro hzongaro approved these changes

@hangshao0 hangshao0 hangshao0 approved these changes

Assignees

@hangshao0 hangshao0

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.
3 participants
@theresa-m @hangshao0 @hzongaro