From 30c15a098828cc881ceb90d01f910d792f61ad18 Mon Sep 17 00:00:00 2001 From: Marius Vollmer Date: Mon, 4 Sep 2023 11:38:18 +0300 Subject: [PATCH] Second pixel testing update --- _posts/2023-09-01-pixel-mocking.md | 171 +++++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 _posts/2023-09-01-pixel-mocking.md diff --git a/_posts/2023-09-01-pixel-mocking.md b/_posts/2023-09-01-pixel-mocking.md new file mode 100644 index 00000000..cbb75a3f --- /dev/null +++ b/_posts/2023-09-01-pixel-mocking.md @@ -0,0 +1,171 @@ +--- +title: Pixel testing update II +author: mvo +date: '2023-09-01' +tags: cockpit +slug: pixel-mocking +category: tutorial +summary: 'Mocking the DOM for Better Pixels, plus statistics' +--- + +When testing pixels, one needs to deal with parts of the UI that +change from one run of the test to the next. Things like MAC +addresses, time stamps, and UUIDs are obvious examples. + +The pixel test machinery did have mechanisms for this since the +beginning, and now we have added one more. + +## Ignored pixels + +My initial idea was to use the alpha channel in reference images to +mark specific pixels where the reference doesn't need to match the +current pixels. I did indeed think that we would open GIMP and +carefully paint the areas that we expect to be unstable. That never +happened, obviously. The code to support this is still there, but it +was clear very early on that this is too much work. + +So we had the `ignore` argument for `assert_pixels`. With this, you +could specify right in the tests which DOM elements were allowed to +differ from one run to the next. This was much easier to maintain and +made it much more obvious what was going on. + +However, different element content might not only change the pixels +inside the element itself, it might also change the size of the +element, and that in turn might change the layout of other +elements. + +For example, if a DOM element that should be ignored gets smaller from +one run to the next, the old pixels in the reference image that are +now outside of this element (but used to be inside) will now count as +a difference. + +In reality, this is much less of a problem than it might seem: We +usually compare big containers for pixel tests, such as a whole dialog +or top-level panels. These larger elements are not supposed to change +size when their content changes. + +However, their internal layout might still be affected: The most +stubborn example is maybe table layout. If the size of the content of +a cell changed at all, the widths of all table columns might +change. We ended up ignoring most content of tables, which is a bit +unsatisfactory. + +## Fake pixels + +Of course, some large elements do change their size depending on their +content, like pop-ups and tooltips. + +So in order to be able to reliably pixel test those and the tables, we +started exploring another approach: Replacing the real data in the UI +with "mock data". Instead of ignoring a UUID, we would inject a +specific fake one into the UI. + +This is conceptually simple, but I got a serious case of "perfection +is the enemy of the good". I was imagining all kinds of theoretical +problems, and I wanted to avoid them all with a bullet proof +implementation. This ultimately failed, but the journey might be worth +telling. + +The main issue is that the DOM is "owned" by React, and modifiying it +without involving React seemed like asking for trouble. React might +decide to render just when the tests had inserted their mock +data. Also, as it turned out later, React keeps references to the +nodes it has created and will crash if it finds the DOM in an +unexpected state. + +So, the first try was to involve the Cockpit UI code itself in the +mocking process and insert the fake data in front of React. This would +be ultimately stable and ultimately flexible. People would write +something like this to implement a mockable React component: + +```js +const Thing = ({ id }) => { + const mock = useMock('thing'); + + return
UUID: {mock.?uuid || thing.uuid}
; +} +``` + +And this to control it from a test: + +```py + b.assert_pixels(..., mock={'thing': { uuid: "4713f4c5-404b-43d6-9df6-d6e7f9d951eb" }}) +``` + +This wasn't popular. We don't want to litter our code with a lot of +stuff that is only used during testing. + +Instead, we really want to write this in a test: + +```py + b.assert_pixels(..., mock={'#uuid': "4713f4c5-404b-43d6-9df6-d6e7f9d951eb" }) +``` + +This means changing the DOM without telling React: The text content of +anything matching `#uuid` would be replaced with the given string, +right in the DOM. + +The first idea was to stop JavaScript execution on the page so that +React didn't have any chance to interfere. But this meant doin all +DOM manipulation with the CDP APIs! That was not a lot of fun because +those APIs are a bit clunky, and I gave up when React started crashing +anyway because of the unexpected DOM changes. I could probably pressed +on and avoided those crashes, but I was actually happy to give up for +some time. + +Using the CDP APIs also meant we couldn't use the nice Sizzle CSS +query syntax that we enjoy in the rest of our test code, and that was +another reason to abandon this route. + +But then we found more pixel testing problems that looked like they +could benefit from mocking. At that point, I was finally ready to say +"screw perfection" and just did the most direct implementation that +would work for the most immediate cases that we wanted to tackle, and +race conditions be damned. + +So the first implementation that landed in main was little more than + +``` +document.querySelector("#uuid").textContent = "4713f4c5-404b-43d6-9df6-d6e7f9d951eb"; +``` + +This worked surprisingly well, and I should have just done this from +the start. + +Later, we did get the expected React crashes, and we had to improve +the mocking code to never remove nodes from the DOM. Now it is careful +to only ever change attributes of nodes of type "TEXT". + +We already need to make sure that React is reasonably quiet when the +tests look into the DOM, so I don't actually expect a lot of trouble +in this regard. And we will anyway only fix actual problems from now +on, not imaginary ones! + + +## Real numbers + +Lets look at some numbers and compare them to [from 19 months +ago](https://cockpit-project.org/blog/pixel-testing-update.html). + +We now have two more layout variants, "dark" and "rtl". These cover +two new features in Cockpit, "Dark Mode" and right to left text +rendering. As with the "mobile" layout, pixel testing these will help +avoid regressions even if us developers rarely use Cockpit in those +modes ourselves. + +The main Cockpit tests now have 72 pixel tests (up from 52), +Cockpit-podman now has 12 (up from 5), and Cockpit-machines has 31 (up +from 20). + +The full set of reference images is now about 20 MiB (up from 10 MiB). + +The repository with all the history of all reference images is now +about 220 MiB, up from only 22 MiB. As a comparison, the main Cockpit +source repository grew to about 200 MiB from 110 MiB in the same time. + +So as expected, the reference image history grows much faster in size +than the code history. It was the right call to keep it out of the +main source repository and arrange its commits so that we can prune +history without affecting the main source repositories at all. Still, +we can probably do another five years of pixel testing without having +to think of actually pruning anything.