ui-smoke phase 3: screenshots, writable config mirrors, homing settle - #4136
ui-smoke phase 3: screenshots, writable config mirrors, homing settle#4136grandixximo wants to merge 28 commits into
Conversation
|
For now the screenshots of the UI are uploaded, and it is up to the reviewers to check, if wanted we could save in source a screenshot to verify against that nothing changed, but then it will be a bit of a bother when anything is supposed to actually change, and for UI with multiple pages, we may want to verify with more than one screenshot at probably more than one resolution? that could take a while, or we can just call it done here, verify UI works with uploaded screenshot is already pretty good, |
fb9f031 to
186c3a3
Compare
|
Added the known-good image comparison. On a clean run each GUI's confirm.png is compared (ImageMagick) against a committed reference.png, and both the shot and a highlighted diff.png are uploaded as CI artifacts. It never fails a test: font/freetype versions differ across distros, so we just record the diff to gather experience first. Even same-machine the 3D preview drifts a few hundred pixels, a handy sanity check. Regenerate locally on a built RIP tree with tests/ui-smoke/_lib/make-references.sh (sets UI_SMOKE_UPDATE_REFERENCE=1). The four baselines were captured on Debian trixie; other distros will differ, which is what we want to observe. Also rebased onto master. @BsAtHome This should fit the shape we discussed in the meeting and by email. |
186c3a3 to
3523187
Compare
bf89855 to
c4de595
Compare
|
Fedora: Have yet to test on Gentoo, still battling networking issues. |
|
I don't really understand the diff images. They have a gray tint and reveal the actual image. I expected them to be nearly or completely black. A (biased) difference is not interesting for which direction it goes. The absolute difference would be more telling. Essentially: for each pixel and color |
|
Anything that differs is bright red, I have not looked at the latest ones yet, if they are mostly greyish means no changes |
|
Tomorrow I will do some builds, and have different screenshots so we can pick a winner, at least now it seems to be working alright, the final look it's just a matter of taste 😉 |
| # (the suite must run unattended), and skip under runtests -u so a | ||
| # user opting out of root tweaks is respected even when invoking the | ||
| # suite from a root shell. Non-root falls back to a cwd "core". | ||
| if [ "$(id -u)" = 0 ] && [ "${RUNTESTS_NOSUDO:-0}" != "1" ]; then |
There was a problem hiding this comment.
Hmm, this makes no sense, checking if we are root and then if RUNTESTS_NOSUDO not doing it.
Here, you can only be root if you use sudo runtests right? However, sudo runtests will always fail.
I would just check for "${RUNTESTS_NOSUDO:-0}" != "1" and then do sudo sysctl
Did you ever test if you get a crashdump if there is a segfault?
|
I missed one, you have to add gdb here to: Suggestion: The same for the image tests, break a gui on purpose and see what happens. Having tests pass is good but having them fail when there is an issue is even better... ;-) Otherwise, the concept looks really good! Sadly I don't have the time right now to do a proper review. |
gdb was only added to .github/scripts/install-deps.sh, which runs on the Ubuntu rip-and-test jobs. The package-arch jobs build and test in debian containers and install build deps via apt-get build-dep, so they never got gdb and the ui-smoke crash dump path had none when a GUI cored there (hdiethelm, PR LinuxCNC#4136 review). Move gdb to debian/control.top.in under <!nocheck>, next to the other test-only deps (xvfb, x11-utils, imagemagick). build-dep installs it for both the rip jobs and the package-arch jobs, so the explicit entry in install-deps.sh is now redundant and dropped.
grandixximo
changed the title
|
@BsAtHome this is gmoccapy current method the icon and time differ this is absdiff method axis current method the preview differs, the fonts I fuzzed a bit cause they were always coming up as positive all red with my reference picture, this is so when running locally you don't get all red 
this is axis with absdiff with slightly different fonts, and the plot 
this is with added fuzz on the absdiff method, only see plot difference 
Both methods express the same thing in a different way, now that you see them next to each other, do you have a preference? |
76f47e8 to
ed7f125
Compare
|
@hdiethelm Moved gdb as suggested. The crashdump did work initially and was tested, but I did not run it again after recent permissions changes, my bad, I did testing again results are as follows Failure path: I injected a real segfault into a GUI and pushed it through CI to watch the actual runners. Good news: it fails fast (the test goes red in seconds, no hang), the Python faulthandler traceback in linuxcnc.err names the crash frame, the watchdog catches it with UI_SMOKE_FAIL: linuxcnc exited before the driver finished, and the failure screenshots still upload. Result line was 291 tests run, 290 successful, 1 failed. Your freeze was most likely the rtapi_app path: when the RT loader dies, linuxcnc hangs bringing HAL up rather than the GUI process exiting, so the launcher-PID watch never fires and only the outer timeout ends it, slowly. The GUI-crash path, which is what the crash dump is for, behaves. Testing it again also exposed and fixed two crash-dump bugs: it could pick up a stale root-owned core from /tmp (gdb then just printed Permission denied), and a non-root run produces no core at all since we will not sudo to set core_pattern. Fixed: only use a readable core from this run, otherwise say so and lean on the Python traceback. The native gdb backtrace stays a best-effort extra for when the suite runs as root. One note from the CI run: because the runners execute the suite non-root, the native backtrace path correctly no-ops and prints no readable core dump; see the Python traceback. The Python traceback is the crash signal CI actually delivers, and it does. |
|
Actually, I like both image diffs (without fuzz) when viewed together. Do we need to choose or can we have them both? |
|
Shouldn't be a problem to have both, can even have multiples fuzz levels, the text rendering I think goes away around 20% fuzz with no fuzz the text will always be almost all red, so it will be harder to spot if any labels were changed |
|
Instead of having them as separate diffs, I could collage everything in a single huge image, with tiles, where each tite is titled sensibly, like original and build next to each other on top, then more tiles for the "a-b diff" and the "red marks the diff" at 0 and 20% fuzz, 6 tiles, 2x3, I think that should give a proper overview, or am I overdoing it? |
|
Combining them in one image will make the lighter parts hide the darker parts (eye accommodation problem). Therefore, having them separate is best. The small text rendering differences are not bright red as long as they are just rendering differences. A real text change would make that much more visible. I think it be good to see small rendering changes because it says something about stability. Therefore, no fuzz would be fine. If we are to look at more differences, then we probably should look at differences between distributions. However, this is not a ui-smoke test problem. This would more be like a "let me see what I broke yesterday" matrix comparison. You'd need to collect images from all distros/versions and then be able to do a local RIP comparison. A different project ;-) |
0fdde41 to
380e419
Compare
grandixximo
changed the title
380e419 to
31c9888
Compare
Now gdb is installed with apt-get build-dep right? This just means everybody that builds linuxcnc will have gdb installed. But gdb is anyway needed to run the test, so fine in my opinion. An alternative would be to add it to the two places in the CI. If anyone has an issue with this, I can send you a patch to install it CI only.
Possibly I was not patient enough.
You can use sudo in CI, it has nopasswd set. Changing: |
…-get The ui-smoke crash dump helper used to apt-get install gdb on demand when a core landed and gdb was missing, which is awkward inside a test run (hdiethelm, PR LinuxCNC#4054 review). Add gdb to the build-essential block in .github/scripts/install-deps.sh so CI has it before any test starts, and drop the in-test apt-get from crashdump.sh: the helper now just checks for gdb and falls back to a 'gdb unavailable' note otherwise. A local developer who wants the native backtrace installs gdb the same way they installed any other build dep.
…or -u The -u option already skips test directories whose control file declares Restrictions: sudo, but a test still running under -u had no way to know its parent was run with -u and could not condition its own root-only side effects on it (hdiethelm, PR LinuxCNC#4054 review). Export RUNTESTS_NOSUDO=1 alongside the existing internal NOSUDO=true so child shells can branch on it. The ui-smoke crash dump helper uses this to skip the global kernel.core_pattern sysctl when -u is in effect, even when the suite is launched from a root shell.
Boots touchy on a 1024x768 screen and fails if its window is larger, guarding against a GUI growing off the display (touchy has no scrolling and has done this). window-fit.sh compares the window geometry to the root window via xwininfo; launch.sh runs it when UI_SMOKE_FIT_CLASS is set and folds a failure into the screenshot path. Needs x11-utils. Requires the touchy window-bounding fix to pass (red on current master).
Two runs of the axis ui-smoke in the same container produced 21665 differing pixels at 5 % fuzz because every antialiased glyph edge shifted subpixel between runs (axis has the most text of the four GUIs). The metric is intentionally informational, but a five-figure AE buries any real drift in noise. 40 % fuzz absorbs font and icon edge jitter and the AE drops to 77 pixels on the same pair, which is the genuine difference floor (subpixel rasterisation in the GL preview). Above 40 % the AE stays at 77. A widget that moves or a label that changes is still flagged loudly.
LinuxCNC#4170 floors the touchy window height at 500 px so all notebook tabs are usable on first launch. Refresh the committed confirm reference so the ui-smoke comparison matches the new patched layout instead of the old ~270 px window. The reference is the exact image CI produces (taken from the ui-smoke-screenshots artifact), so it carries CI's font stack and the comparison starts from a zero baseline; a reference rendered on a developer machine has slightly different text metrics and shifts the whole layout a few pixels, ghosting every label in the diff. Also drop the fit screen size from 1024x768 to 1024x600: 1024x600 is the 7" touchscreen panel touchy actually targets, and the new 500 px floor still leaves room (touchy opens 984x500 inside 1024x600 so the window-fit assertion passes).
Replace the confirm references with the exact images CI produces (from the ui-smoke-screenshots artifact). The previous baselines were rendered on a developer machine and differed from CI by a few hundred pixels of font-edge jitter; adopting CI's own output makes the diff start from a zero baseline so a real regression stands out instead of hiding under the per-distro rendering noise floor.
gdb was only added to .github/scripts/install-deps.sh, which runs on the Ubuntu rip-and-test jobs. The package-arch jobs build and test in debian containers and install build deps via apt-get build-dep, so they never got gdb and the ui-smoke crash dump path had none when a GUI cored there (hdiethelm, PR LinuxCNC#4136 review). Move gdb to debian/control.top.in under <!nocheck>, next to the other test-only deps (xvfb, x11-utils, imagemagick). build-dep installs it for both the rip jobs and the package-arch jobs, so the explicit entry in install-deps.sh is now redundant and dropped.
…race Testing the crash path (segfault injected into a GUI) surfaced two things in the crash dump helper. First, it globbed /tmp/core* and picked up a stale, root-owned core from an unrelated run, so gdb printed only 'Permission denied'. Second, the non-root case (CI, and local runtests -u) never produces a core at all: we will not sudo to point kernel.core_pattern at a writable dir, so nothing lands. Restrict the core search to a core the kernel wrote into our own fresh CORE_DIR, or a relative 'core' in the cwd that postdates arming, and require it to be readable. When there is no such core, say so and point at the Python faulthandler traceback in linuxcnc.err, which names the crash site and is the reliable signal in every environment. The native backtrace stays a best-effort extra for the root case. Verified: an injected GUI segfault now fails the test in ~20s (no hang), logs the Python traceback, and prints a clear 'no readable core dump' note instead of a misleading permission error.
Write two confirm-vs-reference diff images instead of one, both at 0%
fuzz so nothing is hidden:
diff.png red highlight over a faded reference (ImageMagick compare)
diff-abs.png absolute per-channel |b - a|, black where equal and
bright where changed
The absolute difference is unbiased (direction does not matter) and
preserves magnitude, which the red-on-faded-original loses. Both are
uploaded as CI artifacts (gcc and clang) and gitignored locally. The
AE pixel count stays informational; the comparison never gates a test.
Replace the axis and gmoccapy reference.png with the confirm shots from a clean CI run (run 27657962145 on this branch tip), so the committed known-good baseline is the CI ground truth everyone compares against rather than a developer-machine capture. The references are not a pass/fail gate; pinning them to CI just makes the recorded diffs meaningful across distros.
The offscreen grab could catch qtdragon in different startup states
between runs: the 3D preview at different zooms and the gcode view at
different scroll positions. Two distinct causes:
- The preview computes its zoom once, at file-load, from the live
widget size (glnav.py set_view_p reads winfo_width/height) and
never re-fits on resize (qt5_graphics.py resizeGL only resets the
GL viewport). Under offscreen the file load can run before the
layout settles, freezing a stale zoom from a transient size.
- The gcode editor resets to the top on load, then scrolls to the
running line on line-changed; which one the grab catches is timing.
Before grabbing, normalize at the now-final widget size: re-issue the
view fit (set_current_view) so the zoom is recomputed against the fixed
layout, and pin the gcode scrollbar to the top. This removes the large
preview-zoom and scroll swings (previously tens of thousands of
differing pixels). reference.png is captured from CI with this in place.
crashdump_arm gated setting kernel.core_pattern on "id -u = 0", but the suite never runs as root (linuxcnc refuses to start as root), so the pattern was never pointed at our writable dir and the native gdb backtrace never ran, not even on CI where it could. Drop the root check and set core_pattern via sudo, which CI has passwordless; still skipped under runtests -u so a user opting out of root tweaks is respected. Verified on a real host with passwordless sudo: a GUI segfault now writes a core into our dir and the crashdump prints the gdb backtrace (the crash frame, ffi_call, g_main_loop_run, gtk_main) plus all-threads. In a docker container /proc/sys is read-only so the arm no-ops there and the run falls back to the Python traceback, as before. Suggested by hdiethelm.
Under CI load a GUI can still be initializing when the confirm shot is grabbed: axis has been caught with its Manual Control widgets not yet built and the program not loaded, and separately with the Max Velocity slider still syncing from the [DISPLAY] limit (300) down to the [TRAJ] limit (240). Replace the fixed pre-grab sleep with screenshot_grab_settled, which re-grabs the root window until two consecutive frames match within a threshold (absorbing 3D-preview anti-aliasing and a ticking clock) or a timeout hits, then keeps the last frame. Offscreen qtdragon normalizes itself in the grab shim, so it still grabs once. Verified in the CI container: axis and gmoccapy settle in normal time and produce a fully-built shot.
With the settle grab in place, axis reliably captures its post-clamp state: the Max Velocity slider shows 240 in/min (the [TRAJ] limit) rather than the 300 transient from [DISPLAY] that the old grab sometimes caught. Both the gcc and clang CI jobs now agree on 240, so adopt that confirm (run 27734628992) as the reference.
Arming kernel.core_pattern via sudo on every run is not friendly to someone running the suite outside a VM, since it leaves that global setting changed afterwards. Gate the whole crash dump behind a new runtests -d (ENABLE_CRASHDUMPS=1), off by default, and pass -d in the three CI invocations. Without -d, crashdump_arm/report no-op: no sudo, no core_pattern change. The Python faulthandler traceback is unaffected and remains the always-present crash signal. Per hdiethelm review.
Combining -d (crash dumps) with -u (no sudo) asked crashdump_arm to prepare a core dir but skip the sudo that points core_pattern at it, so no readable core could ever land there: a contradiction. Drop the RUNTESTS_NOSUDO check from crashdump_arm (now that -d implies sudo is allowed it always arms) and error out in runtests.in if -d and -u are given together. RUNTESTS_NOSUDO had no other consumer, so its export is removed too. Per hdiethelm review.
fab2bfe to
f979c10
Compare
Current CI (gcc and clang, run 31063545418) settles the axis Max Velocity readout at 300 in/min again. The 240 vs 300 split is a startup race: axis initializes the slider to the [DISPLAY] maximum (5 in/s) and the Tk scale -command sends c.maxvel(5); whether that NML write lands before or after linuxcncsvr formats the command buffer decides if motion's velocity limit stays at the [TRAJ] default (4 in/s, readout clamps to 240) or moves to 5 (300). Both end states are stable, so the settle grab cannot catch the loser. A separate PR aligns [TRAJ]MAX_LINEAR_VELOCITY with [DISPLAY] and the joints at 5 to make the settled value deterministic; adopt the 300 confirm in the meantime.
| if [ -n "$core" ] && command -v gdb >/dev/null 2>&1; then | ||
| echo "=== crash: native backtrace ($core) ===" | ||
| # gdb reads the core; pull it in if missing, only when root. | ||
| if ! command -v gdb >/dev/null 2>&1 && [ "$(id -u)" = 0 ]; then | ||
| apt-get install -y -q gdb >/dev/null 2>&1 || true | ||
| fi | ||
| if command -v gdb >/dev/null 2>&1; then | ||
| # "bt" first: gdb auto-selects the faulting thread on a SIGSEGV | ||
| # core. "thread apply all bt" after gives the rest. | ||
| gdb -batch -nx \ | ||
| -ex "bt" \ | ||
| -ex "echo \n=== all threads ===\n" \ | ||
| -ex "thread apply all bt" \ | ||
| "$(command -v python3)" "$core" 2>&1 | head -400 | ||
| else | ||
| echo "(gdb unavailable; core left at $core)" | ||
| fi | ||
| # "bt" first: gdb auto-selects the faulting thread on a SIGSEGV | ||
| # core. "thread apply all bt" after gives the rest. | ||
| gdb -batch -nx \ | ||
| -ex "bt" \ | ||
| -ex "echo \n=== all threads ===\n" \ | ||
| -ex "thread apply all bt" \ | ||
| "$(command -v python3)" "$core" 2>&1 | head -400 |
There was a problem hiding this comment.
Aren't all modern systems using systemd and coredumpctl? You don't see the core dump and need to run coredumpctl debug to access/debug it.
| # Python traceback. | ||
| sudo sysctl -w "kernel.core_pattern=$CORE_DIR/core.%e.%p" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
It is problematic to have sudo in here.
|
Two (extra) points:
|
f8fbacb to
4aeef18
Compare
GSTAT only emits 'file-loaded' if the poll that first sees stat.file change finds the interp IDLE. A client doing program_open + AUTO_RUN back to back starts the program within ms, so every 100 ms poll sees the file with the interp busy, the remap guard swallows the change, and the signal is never emitted: gmoccapy sits at 'No Program loaded' with an empty editor and preview after an externally driven run, and whether it does depends on poll timing. Emit a skipped change on the first poll with the interp back at IDLE. Remap protection unchanged: merge() ignores the file name at call level != 0.
GSTAT emits 'file-loaded' only from a poll that sees the file change with the interp IDLE. AUTO_RUN immediately after program_open makes the GTK GUIs miss it, so gmoccapy showed 'No Program loaded' in the confirm shots depending on poll timing. Settle 0.4 s between open and run so the shot deterministically shows the loaded program.
…ed pattern crashdump.sh changed kernel.core_pattern through sudo and never restored it: a test script mutating a global kernel setting, and one that fights systemd-coredump hosts, where the core is only reachable through coredumpctl. Drop sudo and the sysctl from the test. crashdump_arm now only raises the core limit; crashdump_report collects the core from wherever the system put it: CORE_DIR (which the CI workflow points core_pattern at with a job-level sudo sysctl on the disposable runner), a plain-file pattern's ./core, or coredumpctl on systemd hosts. The -d/-u conflict in runtests is gone with the sudo, so -d stays a plain opt-in.
4 participants
Builds on the merged phase 2 (#4054). This phase makes the ui-smoke runs more diagnosable and adds writable-config handling so the GUIs start cleanly headless.
What's here:
hide_startup_messsageso the "Important change(s)" modal stays hidden (the same effect as ticking "Don't show this again"). This is the headless-start fix discussed in gmoccapy: modal startup "Important change(s)" dialog blocks headless / non-interactive startup #4072.Scope note: the touchy window-fit smoke test is intentionally held back on a separate branch, since it depends on the touchy fitting changes in #4131 which are not merged yet. I will send it as a follow-up once #4131 lands.