python3Packages.pywebview: build fix for tests #353833
Conversation
|
Mirroring @benaryorg's comment from #353686.
|
|
Did a quick test by depending on |
|
GGG-KILLER
added
the
12.approvals: 1
Fixes NixOS#353686 Basically the *tests/run.sh* used upstream has a few rough edges and this replaces it with a smoother version. An issue was also opened on the upstream project to maybe get this smoothed out generally. Story time for those who are curious. Basically upstream uses this as a script to call for the CI pipeline where [the builds seem to run smoothly in appveyor](https://ci.appveyor.com/project/r0x0r/pywebview/builds/50791017). However the general structure of the script iterates over the files, which in earlier versions had been done by collecting the list of tests via pytest itself, which replaced the earliest implementation which was a file hard-coding all the tests to run. The latter had the benefit of being able to disable tests by commenting them out on our end, however the new version, at least for our purpose, is just a more complicated version of running pytest against the entire thing. We can't just use plain pytest however (which'd presumably be supported by nixpkgs infra already) because we still need to shove the Qt and xvfb-run shims in between. So with running pytest as a single command we are now (with this commit) able to specifically disable tests that we know to be flakey using regular pytest means. With the Qt wrapper function passing extra args to *makeWrapper* we can use the extra flags to pass everything we need, and with the env invocation we avoid polluting the build environment so that the *checkPhase* itself doesn't change the output. Now on to the actual failing tests, apparently those happened to be related to relative paths which use an internal HTTP server to be served (for absolute paths this is optional), and getting rid of the cwd shenanigans which were required by the upstream version of the script (since it globbed on the current directory) means that somehow pytest now runs these tests without changing directory in a subprocess so the asset used for testing is properly accessible (before this change one could "fix" the tests by changing to an absolute path in the tests). Signed-off-by: benaryorg <[email protected]>
benaryorg
force-pushed
the
zhf-24_11-2
branch
from
5156906
to
bdfa0f0
Compare
wegank
removed
the
12.approvals: 1
There was a problem hiding this comment.
nixpkgs-review result
Generated using nixpkgs-review.
Command: nixpkgs-review pr 353833
x86_64-linux
✅ 6 packages built:
- jellyfin-mpv-shim
- jellyfin-mpv-shim.dist
- python311Packages.pywebview
- python311Packages.pywebview.dist
- python312Packages.pywebview
- python312Packages.pywebview.dist
| # QStandardPaths: XDG_RUNTIME_DIR not set | ||
| export XDG_RUNTIME_DIR=$HOME/xdg-runtime-dir | ||
| # a Qt wrapper is required to run the Qt backend | ||
| # since the upstream script does not have a way to disable tests individually pytest is used directly instead |
There was a problem hiding this comment.
It'd be great if you made upstream aware of this so that they can both fix the tests and provide a mechanism for disabling individual tests. Please link the issue if you do.
There was a problem hiding this comment.
(While you're at it you could also ask them to make these faster and/or parallelise them; my god are these slow.)
There was a problem hiding this comment.
I already had the upstream issue opened to figure out why the run.sh is structured the way it is, and I only just got a response that I'll have to ponder for a bit.
Considering that the tests most likely do actual graphical things (otherwise the xvfb-run wouldn't be needed) I wouldn't get my hopes up about parallel execution.
It is on my todo list.
Fixes #353686
Basically the tests/run.sh used upstream has a few rough edges and this replaces it with a smooth version. An issue was also opened on the upstream project to maybe get this smoothed out generally.
Story time for those who are curious.
Basically upstream uses this as a script to call for the CI pipeline where the builds seem to run smoothly in appveyor. However the general structure of the script iterates over the files, which in earlier versions had been done by collecting the list of tests via pytest itself, which replaced the earliest implementation which was a file hard-coding all the tests to run. The latter had the benefit of being able to disable tests by commenting them out on our end, however the new version, at least for our purpose, is just a more complicated version of running pytest against the entire thing. We can't just use plain pytest however (which'd presumably be supported by nixpkgs infra already) because we still need to shove the Qt and xvfb-run shims in between. So with running pytest as a single command we are now (with this commit) able to specifically disable tests that we know to be flakey using regular pytest means. Side note here though: I personally have no idea how the wrapQtApp magics work internally (despite reading the hook), so I kept the wrapper part verbatim. Now on to the actual failing tests, apparently those happened to be related to relative paths which use an internal HTTP server to be served (for absolute paths this is optional), and getting rid of the cwd shenanigans which were required by the upstream version of the script (since it globbed on the current directory) means that somehow pytest now runs these tests without changing directory in a subprocess so the asset used for testing is properly accessible (before this change one could "fix" the tests by changing to an absolute path in the tests).
ZHF: #352882
One more thing: please someone thoroughly nitpick the heck out of this change of mine because I sure as hell don't feel confident with that code. It feels unclean and needlessly complicated with the heredoc, but I don't know how to pass the Qt stuff into the xvfb-run so.… if maintainers are fine with this quality I'm okay with it, but if anyone has even a slightly better idea on how to do this, go ahead.
Meanwhile I'll try to get a better version of the run.sh into upstream so that maybe we can even pass in our own pytest options while keeping the upstream stuff (i.e. being able to pass the
--deselectfor a disabled test while keeping anything upstream might add), but that may be late for ZHF.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.