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.

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

sound: add tests to Alsa and other test stuff #579

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

epilys
Copy link
Member

@epilys epilys commented Dec 13, 2023

Summary of the PR

add tests to Alsa and other test stuff

Before

FilenameFunction CoverageLine CoverageRegion CoverageBranch Coverage
audio_backends.rs 100.00% (6/6) 92.00% (23/25) 75.00% (9/12)- (0/0)
audio_backends/alsa.rs 0.00% (0/21) 0.00% (0/519) 0.00% (0/375)- (0/0)
audio_backends/null.rs 100.00% (7/7) 100.00% (33/33) 84.62% (11/13)- (0/0)
audio_backends/pipewire.rs 95.45% (21/22) 63.49% (313/493) 48.20% (107/222)- (0/0)
audio_backends/pipewire/test_utils.rs 100.00% (6/6) 91.11% (82/90) 76.47% (26/34)- (0/0)
device.rs 61.22% (30/49) 74.59% (690/925) 53.11% (205/386)- (0/0)
lib.rs 75.61% (31/41) 84.18% (149/177) 70.21% (66/94)- (0/0)
main.rs 60.00% (6/10) 68.75% (22/32) 70.59% (12/17)- (0/0)
stream.rs 81.82% (36/44) 96.53% (362/375) 82.73% (91/110)- (0/0)
virtio_sound.rs 100.00% (60/60) 100.00% (191/191) 100.00% (99/99)- (0/0)
Totals 76.32% (203/266) 65.21% (1865/2860) 45.96% (626/1362)- (0/0)

After

FilenameFunction CoverageLine CoverageRegion CoverageBranch Coverage
audio_backends.rs 100.00% (8/8) 100.00% (39/39) 100.00% (17/17)- (0/0)
audio_backends/alsa.rs 88.64% (39/44) 73.56% (704/957) 48.94% (208/425)- (0/0)
audio_backends/null.rs 100.00% (8/8) 100.00% (38/38) 100.00% (14/14)- (0/0)
audio_backends/pipewire.rs 95.65% (22/23) 63.86% (318/498) 51.57% (115/223)- (0/0)
audio_backends/pipewire/test_utils.rs 100.00% (6/6) 91.11% (82/90) 76.47% (26/34)- (0/0)
device.rs 61.54% (32/52) 76.67% (723/943) 60.35% (242/401)- (0/0)
lib.rs 78.72% (37/47) 87.39% (194/222) 76.58% (85/111)- (0/0)
main.rs 63.64% (7/11) 72.97% (27/37) 72.22% (13/18)- (0/0)
stream.rs 84.09% (37/44) 97.87% (367/375) 89.09% (98/110)- (0/0)
virtio_sound.rs 100.00% (60/60) 100.00% (191/191) 100.00% (99/99)- (0/0)
Totals 84.49% (256/303) 79.14% (2683/3390) 63.15% (917/1452)- (0/0)

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Error event HandleUnknownEvent was not detected correctly, since we
first assume `device_event` is correct, use it as an index value to
access `vrings` slice, possible panic, and then match on the `queue_idx`
from the `vring`.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
The init_logger() function gets compiled in tests only, and serves to
initialize logging only once per test thread.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
stop() was not returning an error if stream_id was invalid, it was
succeeding.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

CC @dorindabassey @MatiasVara

@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

Descriptor/buffer logic is missing from tests, hence the low scores.

@stefano-garzarella
Copy link
Member

@epilys thanks for this work!

ALSA tests are failing in the CI. From a quick glance, it looks like some configuration files are missing.
We also need to update staging/coverage_config_x86_64.json

@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

@stefano-garzarella oops, I probably need to use the null device instead of default. Will run the tests locally and re-push.

@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

That was exhausting to figure out, hope it works... Everytime the CI takes 10x longer than writing code!

@epilys epilys force-pushed the alsa-tests branch 2 times, most recently from 2627e19 to d9cfce7 Compare December 13, 2023 15:31
@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

Ok this is mildly annoying. The problem is there's no alsa card in the container. Fine, no problem: define one that writes to /dev/null. Then pipewire breaks because it also uses the alsa configuration. I'll leave it at that for today, if anyone has a good idea let me know.

@stefano-garzarella
Copy link
Member

It looks like only audio_backends::tests::test_alloc_audio_backend is failing, the other pipewire tests are working.
Should we add let _test_harness = PipewireTestHarness::new() in that new test?

@epilys
Copy link
Member Author

epilys commented Dec 13, 2023

It looks like only audio_backends::tests::test_alloc_audio_backend is failing, the other pipewire tests are working. Should we add let _test_harness = PipewireTestHarness::new() in that new test?

Yeah in my tiredness I didn't notice that, only after posting...

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
@MatiasVara
Copy link
Contributor

@epilys Thanks for this work. LGTM.

@stsquad
Copy link
Collaborator

stsquad commented Dec 14, 2023

Whats with the branch coverage being 0/0 despite increasing line coverage - is this a tooling bug?

@epilys
Copy link
Member Author

epilys commented Dec 14, 2023

@stsquad rustc doesn't support it yet rust-lang/rust#79649

Test whatever we can without actually playing/recording everything; this
would require integration tests, not unit tests.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
This increases test coverage.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Add type safe enum to use instead of raw u16 values, which we have to
validate every time we use them.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
@epilys epilys enabled auto-merge (rebase) December 14, 2023 17:44
@epilys epilys merged commit 3ca50b7 into rust-vmm:main Dec 14, 2023
2 checks passed
@epilys epilys deleted the alsa-tests branch December 14, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants