Add smoke test suite #62

pabs3 · 2022-02-13T05:56:48Z

Uses mpv, mpv-mpris, playerctl, sound-theme-freedesktop, bash,
dbus-run-session, xvfb, xauth, jo, jq, socat and awk.

Initial tests include checking metadata, pausing a playing mpv, playing a
paused mpv, toggling mpv between playing and pausing, stopping a playing mpv.

Add enough parameters to make it flexible and useful for testing already
installed mpv mpris plugins, which will primarily be useful for distros.

Document the test requirements and parameters in the README.md file.

Should help prevent breakage and ensure mpv compatibility.

This has been adapted from the Debian autopkgtests that I wrote:

https://sources.debian.org/src/mpv-mpris/unstable/debian/tests/

test/env

test/setup

olifre · 2022-02-13T18:04:08Z

After changing the parts I commented on, I can confirm the tests run fine in case included in Gentoo package tests, too. Great work 👍 .

pabs3 · 2022-02-14T05:56:35Z

Another review would be good, I've made significant improvements to the pull request, including making it check for errors on stderr for each of the tests and making the tests run in parallel (with GNU make output grouping).

test/Makefile

olifre · 2022-02-14T10:30:57Z

@pabs3 That looks great! I currently get empty ipc.output.json in all cases for some reason (then of course leading to all tests failing), I will investigate this more in-depth later tonight.

olifre

This looks great, apart from the version strangeness with socat and the missing \ in one place and a small typo I did not find any issues 👍 .
Since I'm also only a contributor, of course it's up to @hoyon to perform a real review and merge. I think some small extension (in a later commit) could be to also run make test (after installing all dependencies) in the GitHub action workflow so the tests run after every commit / PR.

test/setup

README.md

pabs3 · 2022-02-14T15:45:49Z

Fixed a few more issues. I think it is ready for a final review before merging.

At this time I don't know enough about GitHub actions to integrate the tests with that, but that can happen at a later stage.

hoyon · 2022-02-14T16:21:41Z

Thanks so much for this @pabs3!!!

I haven't been able to review this properly yet but I've checked out the branch and run make test and get this error:

+ exec env MPV_MPRIS_TEST_NAME=./play XDG_RUNTIME_DIR=. TEMPDIR=. TMPDIR=. TEMP=. TMP=. dbus-run-session -- xvfb-run --auto-servernum --auth-file ././play.Xauthority ./play
xvfb-run: unrecognized option '--auth-file'
test play: stderr not empty!
test play: start of stderr ---------------------
xvfb-run: unrecognized option '--auth-file'
test play: end of stderr -----------------------
test play: stderr not empty!
test play: failed with exit code 1
make[1]: *** [Makefile:18: play] Error 1
make[1]: Leaving directory '/home/hoyon/code/mpv-mpris/test'
make: *** [Makefile:56: test] Error 2

Am I missing something? I'm using arch linux and man xvfb-run does mention the --auth-file flag.

At this time I don't know enough about GitHub actions to integrate the tests with that, but that can happen at a later stage.

I'd definitely want that. IMO there's not much value in a test suite if it's not being run automatically on every commit.

olifre · 2022-02-14T16:38:12Z

Am I missing something? I'm using arch linux and man xvfb-run does mention the --auth-file flag.

It seems the flag is listed on the manpage provided for version 21.1.3-2 in Arch here: https://man.archlinux.org/man/xvfb-run.1.en
Which version are you using?

olifre · 2022-02-14T16:39:14Z

Fixed a few more issues. I think it is ready for a final review before merging.

Just a quick reply: On Gentoo, the new code runs like a charm without any changes even in the testing sandbox used by Portage (Gentoo's package manager) 👍 .

hoyon · 2022-02-14T16:52:18Z

It seems the flag is listed on the manpage provided for version 21.1.3-2 in Arch here: man.archlinux.org/man/xvfb-run.1.en Which version are you using?

It seems like the arch linux xvfb-run script has a number of issues. I've managed to manually patch the script to get the tests to pass but will need to find some time to investigate deeper then report upstream.

olifre · 2022-02-14T19:04:32Z

At this time I don't know enough about GitHub actions to integrate the tests with that, but that can happen at a later stage.

I'd definitely want that. IMO there's not much value in a test suite if it's not being run automatically on every commit.

Here's a trivial commit adding this, that was easier than expected:
olifre@eba11c2

Feel free to pull it, add it here, or I can also open a PR after this one is merged 😉.

pabs3 · 2022-02-15T02:23:20Z

@hoyon if I switch from --auth-file to -f, will that work on Arch Linux?

I added a few more fixes:

added the patch from @olifre and fixed it to install mpv too
moved --output-sync=target to MAKEFLAGS in test/Makefile
moved the contents of the tests targets in test/Makefile to a test/wrapper script, tons of shell in Makefiles looks ridiculously ugly to me
switched to a better jq command-line, recommended to me on their IRC channel
fixed a shellcheck warning in the wrapper script
added a GitHub check that the build process doesn't modify files, that .gitignore is correct and make clean is correct.
fixed the README to reference dbus instead of dbus-daemon, since that only appears in Debian bookworm and later, while just dbus is available in all releases and pulls in dbus-daemon in bullseye.

pabs3 · 2022-02-15T02:39:29Z

One more fix: avoid ././ in the Xauthority path.

…

-- bye, pabs https://bonedaddy.net/pabs3/

hoyon

Apologies for the delay!

Some minor comments but thanks again for this!

test/setup

hoyon · 2022-02-18T18:14:23Z

test/env

+TEMP="$MPV_MPRIS_TEST_TMP" \
+TMP="$MPV_MPRIS_TEST_TMP" \
+dbus-run-session -- \
+xvfb-run --auto-servernum --auth-file "$MPV_MPRIS_TEST_XAUTH/$(basename "$1").Xauthority" \


Replacing this with -f does indeed work in arch linux. I've reported the bug upstream.

Fixed, added a comment about it.

What is the bug link?

https://bugs.archlinux.org/task/73865

Added to the patch. Looks like it is fixed now so maybe I can switch back to the long option though?

test/Makefile

pabs3 · 2022-02-20T02:44:41Z

Updated to add an xvfb log too.

Uses mpv, mpv-mpris, playerctl, sound-theme-freedesktop, bash, dbus-run-session, xvfb, xauth, jo, jq, socat and awk. Initial tests include checking metadata, pausing a playing mpv, playing a paused mpv, toggling mpv between playing and pausing, stopping a playing mpv. Add enough parameters to make it flexible and useful for testing already installed mpv mpris plugins, which will primarily be useful for distros. Document the test requirements and parameters in the README.md file. Should help prevent breakage and ensure mpv compatibility.

Ensures that the build process does not modify any files, that the .gitignore file covers all files and that the clean process removes all files not in git.

hoyon

Tests all pass when running in parallel now!

Thanks again for this work. I'll merge it in now!

pabs3 · 2022-02-20T23:51:59Z

Thanks for merging. Any particular reason you do squashes instead of merges or rebases? Can we get a new release for the cleanups and test suite?

…

-- bye, pabs https://bonedaddy.net/pabs3/

olifre reviewed Feb 13, 2022

View reviewed changes

test/env Outdated Show resolved Hide resolved

olifre reviewed Feb 13, 2022

View reviewed changes

test/setup Outdated Show resolved Hide resolved

pabs3 force-pushed the add-tests branch 3 times, most recently from 58e614d to 6648411 Compare February 14, 2022 05:56

olifre reviewed Feb 14, 2022

View reviewed changes

test/Makefile Outdated Show resolved Hide resolved

olifre reviewed Feb 14, 2022

View reviewed changes

test/setup Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

pabs3 force-pushed the add-tests branch 2 times, most recently from 5e8f72f to 0b99f23 Compare February 14, 2022 15:43

pabs3 force-pushed the add-tests branch from 0b99f23 to 67ecf68 Compare February 15, 2022 02:22

pabs3 force-pushed the add-tests branch 2 times, most recently from b21e607 to 8270b2b Compare February 15, 2022 02:37

hoyon requested changes Feb 18, 2022

View reviewed changes

pabs3 force-pushed the add-tests branch 2 times, most recently from 2678374 to d805516 Compare February 20, 2022 02:35

pabs3 and others added 3 commits February 20, 2022 11:29

Add test suite to GitHub actions workflow

f9e8af3

Check for modified files, untracked files and uncleaned files

907d19a

Ensures that the build process does not modify any files, that the .gitignore file covers all files and that the clean process removes all files not in git.

pabs3 force-pushed the add-tests branch from d805516 to 907d19a Compare February 20, 2022 03:30

hoyon approved these changes Feb 20, 2022

View reviewed changes

hoyon merged commit 72945f9 into hoyon:master Feb 20, 2022

pabs3 deleted the add-tests branch February 20, 2022 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add smoke test suite #62

Add smoke test suite #62

pabs3 commented Feb 13, 2022

olifre commented Feb 13, 2022

pabs3 commented Feb 14, 2022

olifre commented Feb 14, 2022

olifre left a comment •

edited

pabs3 commented Feb 14, 2022

hoyon commented Feb 14, 2022

olifre commented Feb 14, 2022

olifre commented Feb 14, 2022

hoyon commented Feb 14, 2022

olifre commented Feb 14, 2022

pabs3 commented Feb 15, 2022

pabs3 commented Feb 15, 2022 via email

hoyon left a comment

hoyon Feb 18, 2022

pabs3 Feb 19, 2022

hoyon Feb 19, 2022

pabs3 Feb 20, 2022

pabs3 commented Feb 20, 2022

hoyon left a comment

pabs3 commented Feb 20, 2022 via email

Add smoke test suite #62

Add smoke test suite #62

Conversation

pabs3 commented Feb 13, 2022

olifre commented Feb 13, 2022

pabs3 commented Feb 14, 2022

olifre commented Feb 14, 2022

olifre left a comment • edited

Choose a reason for hiding this comment

pabs3 commented Feb 14, 2022

hoyon commented Feb 14, 2022

olifre commented Feb 14, 2022

olifre commented Feb 14, 2022

hoyon commented Feb 14, 2022

olifre commented Feb 14, 2022

pabs3 commented Feb 15, 2022

pabs3 commented Feb 15, 2022 via email

hoyon left a comment

Choose a reason for hiding this comment

hoyon Feb 18, 2022

Choose a reason for hiding this comment

pabs3 Feb 19, 2022

Choose a reason for hiding this comment

hoyon Feb 19, 2022

Choose a reason for hiding this comment

pabs3 Feb 20, 2022

Choose a reason for hiding this comment

pabs3 commented Feb 20, 2022

hoyon left a comment

Choose a reason for hiding this comment

pabs3 commented Feb 20, 2022 via email

olifre left a comment •

edited