Skip to content

nixos/test-driver: add Machine.record_audio#153916

Draft
Synthetica9 wants to merge 7 commits intoNixOS:masterfrom
Synthetica9:test-driver-audio-record
Draft

nixos/test-driver: add Machine.record_audio#153916
Synthetica9 wants to merge 7 commits intoNixOS:masterfrom
Synthetica9:test-driver-audio-record

Conversation

@Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented Jan 7, 2022

Motivation for this change

Allows you to record audio from a VM and verify that sound actually played.
Depends on #153866

TODO:
  • Write some docs
  • Get ALSA driver to work for QEMU? It's so choppy on my machine it's actually interfering with the proper functioning of the machine (cage threw a warning that "my machine is too slow" 😅)
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Synthetica9 Synthetica9 requested a review from tfc as a code owner January 7, 2022 23:21
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 7, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 7, 2022
@Synthetica9
Copy link
Member Author

@ofborg test pulseaudio.user pulseaudio.system firefox mpd

@Synthetica9
Copy link
Member Author

Gonna draft this until #154168 is merged, so that'll probably take some time. If anyone wants this in earlier, let me know.

@Synthetica9 Synthetica9 force-pushed the test-driver-audio-record branch from a29cf5a to ff63439 Compare January 30, 2022 13:04
bit_depth: int = 16,
channels: int = 2,
) -> Iterator[None]:
filename = add_extension_if_simple_name(filename, "wav")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about simply enforcing that the user provided a sane filename and throwing a nice self-explaining exception otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think something got lost in the rebase, but this is the same logic we use for screenshots. I factored out the function, but didn't use it for screenshot

r"^\[(\d+)\]: Capturing audio\(\d+,\d+,\d+\) to (.*): (\d+) bytes$",
)

def capture_info() -> Optional[Tuple[int, int]]:
Copy link
Contributor

@tfc tfc Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading this function feels like there is a lot of qemu-audio-capture specific domain logic and only a little bit of glue code that calls qemu-stuff and then throws it through the domain-specific code.

If you put the domain-specific part into a pure function, you would make it unit-testable and have test cases that at the same time document the interface that you expect from qemu. what do you think, does this make sense in your opinion, or do my ideas result in more work than value?

Copy link
Member Author

@Synthetica9 Synthetica9 Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it is already pretty testable by calling it in weird conditions (capture already running at the same filename, invalid arguments, "manually" stopping the capture, etc.)

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 30, 2022
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants