Expand j9vmemtest on Z/OS

[AlenBadel]

Expanding the current tests to add cases where J9PORT_VMEM_PAGE_FLAG_PREFER_PAGEABLE may be used.

Signed-off-by: AlenBadel [email protected]

[AlenBadel]

FYI @gita-omr @babsingh @dmitripivkine

[gita-omr]

The change seems to be matching the design in #8671 (comment).

[gita-omr]

Somehow could not assign @babsingh as a reviewer but could you please review?

[pshipton]

Only committers can be assigned as reviewers or as Assignees. After a non-committer adds a comment to a PR, they can be added as Assignee, not sure if they can be assigned a review. They can however approve a review.

[fjeremic]

What is the difference between this PR and eclipse-omr/omr#4918? Seems to be a lot of duplication here.

[AlenBadel]

What is the difference between this PR and eclipse/omr#4918? Seems to be a lot of duplication here.

There's always been duplication in omrvmemtest/j9vmemtest.
Unless there exists a good reason, I don't see why j9vmemtest is needed. Internal sanity currently uses pltest, but that can be removed since omrporttest is also used during omr sanity testing.

[pshipton]

Unless there exists a good reason, I don't see why j9vmemtest is needed. Internal sanity currently uses pltest, but that can be removed since omrporttest is also used during omr sanity testing.

@smlambert any thoughts on this?

[DanHeidinga]

Internal sanity currently uses pltest, but that can be removed since omrporttest is also used during omr sanity testing.

The phrase "Internal sanity currently ..." is concerning. This project cannot depend on IBM's internal testing.

[AlenBadel]

Internal sanity currently uses pltest, but that can be removed since omrporttest is also used during omr sanity testing.

The phrase "Internal sanity currently ..." is concerning. This project cannot depend on IBM's internal testing.

Agreed. It's not a justification, just curious on what the rationale was.

[pshipton]

OpenJ9 pltest runs in the sanity.functional suite.

[babsingh]

it may require effort: pltest can directly use the OMR test code instead of duplicating code, in the future. OR OpenJ9 sanity.functional can run OMR tests in their builds, which will allow us to remove the duplicate pltest code.

[babsingh]

j9vmemTest is very old, and it doesn't follow the newest coding standards: https://github.com/eclipse/omr/blob/master/doc/CodingStandard.md. It will be tiresome to modernize the entire j9vmemTest. At least, the new code should follow the newest coding standards, and the updated function(s) should be modernized to follow the newest coding standard.

The same review also applies to eclipse-omr/omr#4918.

Functionally, the test cases look fine to me.

@AlenBadel I don't review on a regular basis. If you disagree with any of my feedback, please request a second opinion from a senior reviewer (committer). After addressing my feedback, please contact a committer for a final review, launching PR builds and merging.

OpenJ9 committers: @DanHeidinga @pshipton
OMR committers (for eclipse-omr/omr#4918): @youngar @rwy0717

[AlenBadel]

it may require effort: pltest can directly use the OMR test code instead of duplicating code, in the future. OR OpenJ9 sanity.functional can run OMR tests in their builds, which will allow us to remove the duplicate pltest code.

@pshipton
@babsingh

I suggest that we create an issue to investigate removing the redundant parts of pltest, and if it's completely redundant then replacing it with omrporttest. I also suggest that these test cases be added to OMR, and not OpenJ9.

[pshipton]

I suggest that we create an issue to investigate removing the redundant parts of pltest, and if it's completely redundant then replacing it with omrporttest. I also suggest that these test cases be added to OMR, and not OpenJ9.

yes, this makes sense. I'm not sure why we have "duplicate" tests.

[smlambert]

In reply to #8791 (comment) and the befuddling question of why there are duplicate tests, short answer is dev team would not delete them. Long answer, one has to go back in history, to when a bunch of test material was prepped and pushed into the omr project, including pltest -> porttest.

At that time, it was suggested that the internal pltest be removed, but for each of the attempts to remove it there was something blocking their full removal (see Sept 2015 RTC 100752 work item for details), but the last note was citing the fact that there was not full spec coverage in OMR, so the dev team decided these tests could not be removed from the jvm component.

I am glad that the request to remove the duplicate tests may finally happen.

[DanHeidinga]

I'm onboard with deleting duplicate tests provided that we're still running the tests (omr version or openj9 version) in the OpenJ9 build. I don't want to rely on the test having run at OMR as it may run on different machine configurations, with different compilers, etc.

[AlenBadel]

I've went ahead and created the issue to have these portests removed on OpenJ9.
#8876

I'll be closing this PR, and these changes will go into OMR instead.