cockpit
cockpit copied to clipboard
cockpit-project
storage: Use mock values during some pixel tests
Instead of ignoring parts of the DOM that are unpredictable, we replace them with some constant non-sense. This gets rid of the unpredictable content and also produces a predictable layout by avoiding variable sized content.
We don't need to restore the old values; the idea is that React will do that on the next render. But there is a race between setting the mock text content and taking the snapshot. If React decides to render in that window, things will break. Let's see.
We don't need to restore the old values; the idea is that React will do that on the next render. But there is a race between setting the mock text content and taking the snapshot. If React decides to render in that window, things will break. Let's see.
No, I don't think this is the right solution, as we're still setting up a race condition.
The data itself has to change before the DOM. There's no way to know when the page will refresh with React.
No, I don't think this is the right solution, as we're still setting up a race condition.
We could have a global toggle that puts each page into "mock mode" where it will replace certain elements with fake data. This means changes to our code, but that's totally acceptable.
Or, we could try to stop the pages JavaScript via the CDP Debugger.pause() function... but maybe that will also block our own eval_js and we need to use CDP also to poke in the fake data... sounds a bit too involved to me.
We could have a global toggle that puts each page into "mock mode" where it will replace certain elements with fake data. This means changes to our code, but that's totally acceptable.
I was wondering about this and was going to suggest it, but the ramifications could be immense, as so much can change everywhere.
Or, we could try to stop the pages JavaScript via the CDP Debugger.pause() function... but maybe that will also block our own eval_js and we need to use CDP also to poke in the fake data... sounds a bit too involved to me.
I have been looking for ways to pause React's DOM updating instead. The problem isn't with browser DOM updates, but with React updating the DOM, so we might as well try to attack it at that level.
If we can do something like React.RenderUpdates.Pause() (this is totally made up), swap out the string(s) in the DOM, then test, then React.RenderUpdates.Resume() (also made up), then this simple string swap approach would work fine.
Surely, we're not the first to hit this kind of problem. Right?
shouldComponentUpdate() might almost be what we want.
https://reactjs.org/docs/react-component.html#shouldcomponentupdate
Use shouldComponentUpdate() to let React know if a component’s output is not affected by the current change in state or props.
HOWEVER:
This method only exists as a performance optimization. Do not rely on it to “prevent” a rendering, as this can lead to bugs.
and
In the future React may treat shouldComponentUpdate() as a hint rather than a strict directive, and returning false may still result in a re-rendering of the component.
So I guess we should keep looking.
It would also be nice to flip the theoretical pause switch on a React level, not on a component level.
There's ReactDOM.flushSync() (https://reactjs.org/docs/react-dom.html#flushsync) which causes React to render immediately. If we called that in the test code (important: not in production code) before monkeying with the DOM, then it's a way to prevent some of the race conditions. It's a bad hack though; it would be better to pase/resume React's rendering somehow.
Debugger.pause works well enough, but changing the DOM via the CDP API is cumbersome and apparently also unreliable. Hmm.
and apparently also unreliable
No, it's just that querySelectorAll doesn't understand the:contains syntax.
I have been looking for ways to pause React's DOM updating instead.
Yeah, me too, but I didn't find anything.
React renders should not be totally random, there needs to be an event of some kind. During testing, we control all input events. That leaves the WebSocket events. (Idle and timeout events hopefully don't matter...)
So I am reasonably happy with the Debugger.pause approach. Code could be cleaned up, of course.
Arg, now I get this:
RuntimeError: DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
This originates from React, so I am guessing React is unhappy with our changes to the DOM when it tries to re-render. This would be a show-stopper for the "mock without React involvement" approach.
React renders should not be totally random, there needs to be an event of some kind. During testing, we control all input events. That leaves the WebSocket events. (Idle and timeout events hopefully don't matter...)
No, afaik, React updates the DOM during render calls, and there could be a different render call on the page which could trigger it being updated. It's not necessarily tied to input events.
Again: if we cannot fix the data (static IP), then having mock data is probably the more correct approach here. Otherwise we'll constantly be fighting React and having race conditions.
Alright, what about the global "mock mode", where the tests instruct the page to behave differenty just for the benefits of the tests. I think that is a useful idea in general, and allows us to test things that are otherwise hard to set up "for real".
I have pushed a minimal version of this idea that just replaces certain texts, via a new hook.
Alright, what about the global "mock mode", where the tests instruct the page to behave differenty just for the benefits of the tests. I think that is a useful idea in general, and allows us to test things that are otherwise hard to set up "for real".
Yes, that makes sense.
The big issue with the IPv6 addresses is that we need to be able to swap out data, not just add it, so this should be taken into consideration.
Alternative is to be able to not show stuff when in mock mode, so things can be hidden and alternatives can be shown, instead of changing it.
In defense of the general idea of putting fake data into a page while doing pixel tests:
We already have a mechanism to ignore certain DOM elements. This is necessary if there are any elements that have unpredictable and hard to control content, such as UUIDs of newly created things. However, if the unpredictable content varies enough, it will influence the layout of elements around it. Sometimes this influence is a bug; no matter what an element shows, we don't want it to change the layout. Sometimes, it is not a bug, such as with table columns.
The way tables work with Patternfly is that even the smallest change in one column (such as replacing a "9" with a narrower "1") will change the position of all columns, also by a very small amount. This will shift all texts by half a pixel, say, and will look like unreliable text rendering.
In those cases, it helps to make sure that all columns always have a constant, predictable content. Thus, this mocking mechanism can be seen as a more robust version of ignoring content.
If we want to go further and actually test that the layout of the page changes correctly when element content changes, this mocking mechanism is easier and faster than having to set up the environment to produce the desired values.
Pixel tests are supposed to fail for the time being. Let's fix them immediately before merging.
Alright, nobody likes to change the production code, including me and the Coverage report.
Summary of past approaches:
- Changing the DOM without Reacts involvement might be racy, but we can get that done with CDP and Debugger.pause.
- Forcefully changing the DOM and then hoping that React puts its back how it was doesn't work because React will stop working when it finds the DOM in an unexpected state.
So, I'll try to modify the DOM with Debugger.pause and put it back how it was before unfreezing the page. To make this simply, I'll try to get the desired mocking effect by just adding things that can be cleanly taken away again, like display: none, and using :after .
This means changes to our code, but that's totally acceptable.
Nope, that wasn't acceptable. :-)
@mvollmer shall we close this one then as the implementation with the dom manipulation from testlib was prefered?
@mvollmer shall we close this one then as the implementation with the dom manipulation from testlib was prefered?
Yes, let's.
New story in #18772.