sound tests: Add sndstat nvlist ATF test

[christos]

Summary

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[christos]

Fix minor error with nvlist_exists().

[christos]

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

[dev_submerge.ch]

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

How exactly does it fail when a value is not available? Does it print a comprehensible error message? Otherwise we should test explicitly for the existence of the fields through nvlist_exists_*().
About the values, we could certainly check some basic assumptions about them, but for a first go at it I think checking the existence is enough.
Since the test only runs in a meaningful way when there is a sound device, would it be possible to add a virtual one? Maybe userland provided like virtual_oss does?

Very welcome addition to have tests, BTW.

[christos]

In D45901#1046542, @dev_submerge.ch wrote:

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

How exactly does it fail when a value is not available? Does it print a comprehensible error message? Otherwise we should test explicitly for the existence of the fields through nvlist_exists_*().

After some accidental failures I had when testing, I saw that if an nvlist call fails, the test will fail with it (because it core dumps, @markj is this acceptable in a test case?), and if you run kyua report --verbose -r ... you can see what was printed to stderr. Alternatively, if we want to be extra safe, we can actually call nvlist_exists() for every single field, I just found it a bit tedious.

About the values, we could certainly check some basic assumptions about them, but for a first go at it I think checking the existence is enough.
Since the test only runs in a meaningful way when there is a sound device, would it be possible to add a virtual one? Maybe userland provided like virtual_oss does?

With a userland device, we cannot check any of the sound(4)-specific values, which are a big part of what the nvlist provides. Also virtual_oss is not in base. That being said, I think we can create a dummy driver that simply calls pcm_register() with some pre-defined defaults, and test with it.

Very welcome addition to have tests, BTW.

[dev_submerge.ch]

In D45901#1046543, @christos wrote:

In D45901#1046542, @dev_submerge.ch wrote:

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

How exactly does it fail when a value is not available? Does it print a comprehensible error message? Otherwise we should test explicitly for the existence of the fields through nvlist_exists_*().

After some accidental failures I had when testing, I saw that if an nvlist call fails, the test will fail with it (because it core dumps, @markj is this acceptable in a test case?), and if you run kyua report --verbose -r ... you can see what was printed to stderr. Alternatively, if we want to be extra safe, we can actually call nvlist_exists() for every single field, I just found it a bit tedious.

I have no experience with this test framework - given your description I'd vote for explicit nvlist_exists_*() checks, but I'm not your boss ;-)

About the values, we could certainly check some basic assumptions about them, but for a first go at it I think checking the existence is enough.
Since the test only runs in a meaningful way when there is a sound device, would it be possible to add a virtual one? Maybe userland provided like virtual_oss does?

With a userland device, we cannot check any of the sound(4)-specific values, which are a big part of what the nvlist provides. Also virtual_oss is not in base. That being said, I think we can create a dummy driver that simply calls pcm_register() with some pre-defined defaults, and test with it.

Very welcome addition to have tests, BTW.

I think a dummy driver is a good idea, either as a kernel module if tests are allowed to load it, or enabled via sysctl? The tests should be able to run in automated CI builds. If we extend it to work as a loopback device in the future (playback audio fed back to recording channel), we'll get a whole lot of API testing opportunities with that.

[markj]

In D45901#1046543, @christos wrote:

In D45901#1046542, @dev_submerge.ch wrote:

In D45901#1046389, @christos wrote:

I am not really sure if it's meaningful to check each individual value returned by the nvlist_get_* calls. If any of these calls fails, the test will fail as well, so I think just calling them and making sure nothing fails is enough. Any opinions?

How exactly does it fail when a value is not available? Does it print a comprehensible error message? Otherwise we should test explicitly for the existence of the fields through nvlist_exists_*().

After some accidental failures I had when testing, I saw that if an nvlist call fails, the test will fail with it (because it core dumps, @markj is this acceptable in a test case?), and if you run kyua report --verbose -r ... you can see what was printed to stderr. Alternatively, if we want to be extra safe, we can actually call nvlist_exists() for every single field, I just found it a bit tedious.

It's not really the right way to do things, ideally tests would fail with a descriptive message that explains what went wrong. Indeed, nvlist will assert if you ask for a field that isn't present (or has the wrong type I think?), so your approach works, but you should at least add a comment explaining what you're testing.

tests/sys/sound/sndstat.c

4

Missing a copyright and SDPX identifier for this file.

27

Tests should be skipped if some required resource is missing. For something like this, the normal thing to do would be to skip if opening the file raises ENOENT.

[imp]

tests/sys/sound/sndstat.c

27

I think this should be done in the metadat for the test.

[christos]

In D45901#1046672, @dev_submerge.ch wrote:

I think a dummy driver is a good idea, either as a kernel module if tests are allowed to load it, or enabled via sysctl? The tests should be able to run in automated CI builds. If we extend it to work as a loopback device in the future (playback audio fed back to recording channel), we'll get a whole lot of API testing opportunities with that.

Regarding the loopback, I have been thinking a while ago whether it would be a good idea to have an built-in loopback device provided by sound(4), similar to what the -l option of virtual_oss does. This would make recording desktop audio way easier than it currently is I think.

[christos]

• Add SPDX license identifier.
• Add nvlist_exists() calls for all fields (couldn't avoid the ugly macros...).
• Use atf_tc_skip() in case /dev/sndstat is not present.

[christos]

Use dummy driver implemented in D45967 by loading snd_dummy.ko in the testcase
body.

[christos]

tests/sys/sound/sndstat.c

68

sound.ko is loaded with snd_dummy.ko, however I think it's fine to keep this check to make sure snd_dummy.ko itself doesn't remove this dependency at some point.

88

This could be removed since we have loaded snd_dummy.ko, but I would keep this in case the test case is modified not to use snd_dummy.ko.

140

Ditto.

[markj]

tests/sys/sound/sndstat.c

65

This could live in a separate function, since other tests will probably want to use it as well?

203

I don't think tests should bother with this. In practice, tests are run in a dedicated host where it's ok to load whatever you want. And, this will break if tests are run in parallel (e.g., with kyua -v parallelism=4 test).

[christos]

Address Mark's comments.