8275170: Some jtreg sound tests should be marked with sound keyword

[prrace]

This fix adds "headful sound" keywords to a number of javax/sound jtreg tests

The jtreg javax.sound tests are written in such a way that if a needed audio device
or other platform resource is not available, they just exit with a "pass" for the test.
It is as if headful tests just swallowed HeadlessException and issued a pass.
If we allowed that we'd be effectively testing almost nothing of real importance.

Currently almost all sound tests have no keyword like "headful" to indicate they
may need access to a hardware resource to do anything useful so a similar
situation arises there except when someone runs those tests manually on
a local system which has audio devices.

Of course "headful" and "audio device" aren't exactly interchangeable terms
but if tests are allowed to be run - or worse usually or always run - in a situation
where they potentially are on a system without an audio device (a headless VM) or are running in
a session which doesn't have full desktop access - which may in some
cases be how audio device access is granted, then they are more likely
to be running in this scenario where the testing isn't valid.

Another point is that headful tests must be run one at a time - no concurrency because
you can't have multiple tests moving the mouse at the same time
Whereas for headless tests, a test harness may choose to run concurrently - perhaps even in the same VM
When tests that really need access to an audio device are run it is probably safer to consider
the headful attribute as implying one test at a time is best .. granted on a desktop it isn't
usual for a single app to be able to monopolize access to a speaker, but for input devices
that is what you'd generally expect.

By no means am I sure that the tests being updated here are the full set that need tagging.
There are a lot of tests and they are all known to pass on systems with NO audio
devices, so the search has been for tests which call APIs which will definitely
need access to some device in order to do anything useful.
So likely there are more to be added - either by a reviewer pointing them out or by later discovery.
Alternatively we could mark ALL sound tests headful .. but given where we are starting from
that might be erring unnecessarily far in the opposite direction.

By adding both headful and sound keywords it gives options to someone who
wants to identify the tests and perhaps run just tests which need "sound" on some
config they know supports audio but isn't headful.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

• JDK-8275170: Some jtreg sound tests should be marked with sound keyword

Reviewers

• Alexander Zuev (@azuev-java - Reviewer) ⚠️ Review applies to 98d02ae2
• Sergey Bylokhov (@mrserb - Reviewer)
• Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6086/head:pull/6086
$ git checkout pull/6086

Update a local copy of the PR:
$ git checkout pull/6086
$ git pull https://git.openjdk.java.net/jdk pull/6086/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6086

View PR using the GUI difftool:
$ git pr show -t 6086

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6086.diff

[bridgekeeper]

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@prrace The following labels will be automatically applied to this pull request:

• client
• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (b7e0443e)
• 01: Full - Incremental (98d02ae2)
• 00: Full (5439d6c7)

[mrserb]

I am not sure this is the right thing to do, as you pointed out the headless system may or may not have the sound configured, similar to the headful system may not have a sound. I see a lot of headful systems in the cloud where the audio device is not configured, and the opposite where the headless system actually has some audio devices. And this change just makes things complicated by assignee the headful keyword to the unrelated sound system.

Actually, it is the step in the opposite direction that was done when these tests were moved to tier3, at that time the headful keyword was removed from these tests to make tier3 coverage as much as possible.

I am still not sure what problem do you want to solve? If the problem is to run the tests on the system where the sound is configured then just run them there, so it will not be necessary to validate each test does it really need a sound/headful keyword or not, otherwise what is the benefit?

[prrace]

This makes the tests eligible to be run on headful systems.
In practice it seems like ONLY headful systems actually have the sound devices.
The sound keyword isn't understood by anything yet, so adding headful is the only way
we have right now of ensuring these tests are eligible to be run on systems that have the
necessary support.

As I explained in the initial PR description headful is also useful for implying exclusive access.

And without this keyword it means that these tests are ALWAYS run on systems without
sound device support. So for years we've effectively problem listed these tests due to
the tests not having headful and silently passing when there's no sound.
Without headful no one runs these on systems that have the needed device.

So this solves a big hole that we aren't testing this case.

The 2 keywords give flexibility - anyone who wants to filter when its marked headful
and needing sound can do this, but just adding sound right now will solve nothing.

And there are a couple of tests NOT in OpenJDK that already do this so we have precedent - and in a previous life one was added by you and the other approved by you ..

[mrserb]

This makes the tests eligible to be run on headful systems. In practice it seems like ONLY headful systems actually have the sound devices. The sound keyword isn't understood by anything yet, so adding headful is the only way we have right now of ensuring these tests are eligible to be run on systems that have the necessary support.

This is not the right approach, it was made a hard work to eliminate the headful keyword from any tests including sound which are not throw the headless exception or something. The "headful" means that the test will fail otherwise due to UI session missing. We should not abuse it to select some other unrelated tests. Like we do not doing this for printer.

The problem you try to solve is that you have a "bad" system where nothing is configured and the "good" system where UI/sound(what about printer?) are configured. And you want to exclude all tests from the "bad" host and run them on the "good" one by using the headful keyword which is not targeted for this usecase.

As I explained in the initial PR description headful is also useful for implying exclusive access.

Such exclusive access should not be needed by the sound, in fact it was found a few hard too spot concurrency bugs when we start to run the sound tests in parallel.

And without this keyword it means that these tests are ALWAYS run on systems without sound device support.

Is it always just because the sound is not configured on the systems where you run the tests without headful keyword? How it is related to the "headfulness" of the systems?

So for years we've effectively problem listed these tests due to the tests not having headful and silently passing when there's no sound. Without headful no one runs these on systems that have the needed device.

This was done intentionally that everybody who run the tests also run the tests for the sound, moreover it was included the openjdk tier3, before it was accidentally removed from there.

The 2 keywords give flexibility - anyone who wants to filter when its marked headful and needing sound can do this, but just adding sound right now will solve nothing.

Such flexibility exists already, it is possible to run test/jdk/javax/sound on any computer you want and exclude it from the jdk_desktop test run.

And there are a couple of tests NOT in OpenJDK that already do this so we have precedent - and in a previous life one was added by you and the other approved by you ..

If it is not in the OpenJDK then we do not need to discuss it here and that's not actually a precedent. We should investigate each case one by one, the headful keyword does not solve any issue on windows, since all windows are headful(or most of them where we interested in the audio), on macOS the sound subsystem is built-in. And on Linux it is possible to configure sound as well and it will work fine w/o X server.

I think most of the sound tests are written according to the spec which does not required UI, and if it was necessary to make it pass by running on the system which have UI system configured make me think that some product bug was workaround.

[bridgekeeper]

@prrace This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[AlanBateman]

/label remove core-libs

[openjdk]

@AlanBateman
The core-libs label was successfully removed.

[bridgekeeper]

@prrace This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@prrace This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[prrace]

/open

[openjdk]

@prrace This pull request is now open

[azuev-java]

Looks right to me.

[openjdk]

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8275170: Some jtreg sound tests should be marked with sound keyword

Reviewed-by: kizune, serb, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 42 new commits pushed to the master branch:

• 9473c38: 8286057: Make javac error on a generic enum friendlier
• a0f6dd3: 8287121: Fix typo in jlink's description resource key lookup
• bd361bc: 8286376: Wrong condition for using non-immediate oops on AArch64
• 4042dba: 8287138: Make VerifyOption an enum class
• a276cd2: 8287151: Remove unused parameter in G1CollectedHeap::mark_evac_failure_object
• cf57d72: 8287174: Remove deprecated configure arguments
• 6458a56: 8286177: C2: "failed: non-reduction loop contains reduction nodes" assert failure
• 1cd7850: 8287194: build failure on riscv after JDK-8286825
• 15f1583: 8287008: Improve tests for thread dumps in JSON format
• a5caffd: 8286786: [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java still fails
• ... and 32 more: https://git.openjdk.java.net/jdk/compare/087bccfe28c03cb714d46b307e276efca11a4315...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[prrace]

The current version only adds the 'sound' keyword.
So it doesn't carry any of the implications of headful.

[prrace]

/open

[openjdk]

@prrace This pull request is already open

[mrserb]

Looks fine.

[prrace]

/integrate

[openjdk]

Going to push as commit 25669bb.
Since your change was applied there have been 52 commits pushed to the master branch:

• 4518063: 8281682: Redundant .ico files in Windows app-image cause unnecessary bloat
• fdece9a: 8287169: compiler/arguments/TestCompileThresholdScaling.java fails on x86_32 after JDK-8287052
• 25080e0: 8286891: thread_local causes undefined symbol error with XL C
• a10c559: 8287162: (zipfs) Performance regression related to support for POSIX file permissions
• d888c80: 8287165: JFR: Add logging to jdk/jfr/api/consumer/recordingstream/TestOnEvent.java
• fdc147e: 8287139: aarch64 intrinsic for unsignedMultiplyHigh
• 8f0eb5d: 8287158: Explicitly reject unsupported call shapes on macos-aarch64 in mainline
• 0a82c4e: 8287137: Problemlist failing x86_32 tests after Loom integration
• 5974f5f: 8284213: Replace usages of 'a the' in xml
• e0d361c: 8284191: Replace usages of 'a the' in hotspot and java.base
• ... and 42 more: https://git.openjdk.java.net/jdk/compare/087bccfe28c03cb714d46b307e276efca11a4315...master

Your commit was automatically rebased without conflicts.

[openjdk]

@prrace Pushed as commit 25669bb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.