Openjdk-tests: Update playlist to use new disable feature

Created on 10 Mar 2021  ·  7Comments  ·  Source: AdoptOpenJDK/openjdk-tests

👾 Description of the issue

We should update the openjdk/playlist.xml file to use the new disable feature which uses explicit tags in the xml file to indicate which tests are disabled. The old way was to put a comment in the xml file (often starting with <!-- temporarily disable....
see here for an example: https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/openjdk/playlist.xml#L89

which should be changed to

  <disabled>
    <comment>https://github.com/eclipse/openj9/issues/10757</comment>
    <impl>openj9</impl>
  </disabled>

There are many occurrences to fix in the playlist file, best to start with a search for <!-- temp but also good to look carefully through the playlist file for any comment that may reference that a test has been disabled.

Please refer to the readme for how to use the new disable feature:
https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/doc/userGuide.md#manually-exclude-a-test-target

📋 Step by Step

To solve this issue and contribute a fix you should check the following step-by-step list. A more detailed documentation of the workflow can be found here.

  • [ ] Claim this issue: Comment below.
  • [ ] Fork the repository in github by simply clicking the 'fork' button.
  • [ ] Check out the forked repository
  • [ ] Create a feature branch for the issue. We do not have any naming definition for branches.
  • [ ] Commit your changes.
  • [ ] Start a Pull Request.
  • [ ] Done 👍 Ask in comments for a review :)
  • [ ] If the reviewer find some missing peaces or a problem he will start a discussion with you and describe the next steps how the problem can be solved.
  • [ ] You did it 🎉 We will merge the fix in the master branch.
  • [ ] Thanks, thanks, thanks for being part of this project as an open source contributor ❤️

🤔❓ Questions

If you have any questions just ask us directly in this issue by adding a comment. You can join our community chat at Slack. Next to this you can find a general manual about open source contributions here.

good first issue

All 7 comments

I will work on this one. -Harper Moon

Great! @jwnmoon - if you have any questions, you can post them as comments in this issue. :)

<!-- exclude jdk_jdi on openj9: https://github.com/AdoptOpenJDK/openjdk-tests/issues/1071 -->

Hi, if I'm understanding the line above correctly it's excluding the debug interface for openj9, so it could be written using the new disable feature. Is this something that should be changed?

That is correct @jwnmoon ! That is one of several examples in this file that should be updated.

It indicates that the jdk_jdi test has been disabled for openj9 impl:

so the test defined at https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/openjdk/playlist.xml#L857-L885 should be updated from

    <test>
        <testCaseName>jdk_jdi</testCaseName>
        <variations>
            <variation>Mode150</variation>
            <variation>Mode650</variation>
            <variation>Mode1000</variation>
        </variations>
    ...
        <impls>
            <impl>hotspot</impl>
        </impls>
    </test>

to

    <test>
        <testCaseName>jdk_jdi</testCaseName>
                 <disabled>
                     <comment>https://github.com/AdoptOpenJDK/openjdk-tests/issues/1071</comment>
                     <impl>openj9</impl>
                  </disabled>
        <variations>
            <variation>Mode150</variation>
            <variation>Mode650</variation>
            <variation>Mode1000</variation>
        </variations>
...
    </test>

Where the new <disabled> tag is added, and the <impl> tag can be removed, noting that if the impl tag is defined, it means to restrict the test to whatever implementation is listed there. If the impl tag is not defined, it means the test applies to all implementations (which is true, we just need to temporarily exclude it for the openj9 impl).

Do the lines about hotspot implementations need to be deleted on other tests with the disabled tags as well? I assume the tests would apply to all implementations(except for when openj9 is disabled) so the impl tags shouldn't have to be defined, but I'm not feeling too sure.

We only need to change other <test>s in the playlist file, if they have <!-- --> comments that say something has been disabled. Each <test>...</test> is an independent unit. Looking at the file, I search for <!-- and find that these are the tests that need updating:

I just created a pull request! I would appreciate it if you could review what I have.

Was this page helpful?
0 / 5 - 0 ratings