[EuiDataGrid] Fix row height tests and mocks

[weltenwort]

📝 Summary

This refactors the RowHeightUtils tests to fix problems stemming from spies and fake timers. This has been split out of #6028 (#6024).

🕵️‍♀️ Review notes

• I refactored the way the RowHeightUtils receive references to various dependencies. It now receives them as MutableRefObjects in its constructor without the need for several imperative set* methods. This will make it easier to extend and mock in the future.
• The tests for the RowHeightUtils were not working as intended, because they used some jest features incorrectly. I tried to correct for the following problems before adding additional test cases for the new compensateForLayoutShift method:
  • jest.useFakeTimers was called in several places directly in describe blocks, which means they're not bound to the lifecycle of the test cases. As a consequence some tests failed unpredictably depending on the execution order of the describe blocks. Some test cases also were not calling jest.runAllTimers, which caused several setTimeouts not to be executed and lead to several false positive test results.
  • jest.spyOn was also called in describe blocks directly, but their implementation was never restored. That means that some spied-upon function implementations would be replaced by no-ops when the mocks were reset, which in turn lead to false positives and negatives.

[weltenwort]

@constancecchen does this require a changelog entry in your opinion (or according to the EUI changelog policy)?

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6050/

[cee-chen]

[super optional] if you wanted to leave these required props inline, you could also set rowHeightUtils afterwards on line 60 by doing

const requiredProps = {
  // ...
  gridRef: {
    // etc,
  },
  gridItemsRendered: // etc,
  // ...
};
requiredProps.rowHeightUtils = newRowHeightUtils(requiredProps.gridRef, { current: null });

That is a relatively minor preference on my part to reduce test file setup cruft as much as possible (or as much as isn't actually relevant to what we're testing - and in this file row heights aren't really being tested).

[weltenwort]

If that's the style you prefer, I can change it. I'd recommend to treat values immutably as often as possible, though. It usually reduces cognitive load on the reader and allows for more compiler checks and JIT optimizations.

[cee-chen]

Super interesting!! (I love code discussions like this BTW, so you'll have to forgive me for overindulging 😄)

allows for more compiler checks and JIT optimizations.

TIL 🎓 I had thought that instantiating fewer consts/vars vs. just 1 obj would be 'better' in terms of optimization. Compiler checks makes sense though. I know we're splitting hairs at this point though in terms of microperf, but definitely food for me to think about!

It usually reduces cognitive load on the reader

I definitely acknowledge this is very subjective, but my personal 2c is that more cruft/setup at the top of the file is harder for me to read / more cognitive load than otherwise. I prefer to jump straight into the core purpose/logic of the file when possible to try and grasp its intentions. When I come across a var I'll tend to store it in my short-term memory assuming it'll be re-used later and I'll want to try and connect the var definition/reference to where it's being used, so more vars = more things flying around my head to try and remember. So for the test files if I start by seeing a single const requiredProps = ... declaration, I get a sense of "oh these are just the props needed to get the test started", and I'll collapse that obj or skim/skip past it, whereas if I see other vars defined I'll be like "this seems important/will come into play later, I'll try to remember it individually".

Like I said, very much subjective though, forgive my rambling! My comment definitely was optional/not a change request so if you prefer it this way, feel free to leave it

[cee-chen]

@weltenwort After more thorougly reviewing the diff, I'd vote no changelog needed - the small amount of source code being changed could be classified as tech debt only / non-consumer-impacting.

I think I've left all the comments I'm going to, though would def appreciate a second eye from @chandlerprall. My main/only semi-blocking question would be around useLatest - I'm not quite fully seeing/understanding the need for it. Everything else is a minor non blocking comment or question. Thanks again, this is great!! 👏

[weltenwort]

@constancecchen thank you for the thorough review. I tried to respond to your comments inline. Let me know if those explanations make sense.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6050/

[chandlerprall]

Thank you for the tests clean up, and simplifying some code paths as well! Really appreciate it.

Apart from updating rowHeightsUtils to a useState instead of useMemo (already captured in a thread), everything LGTM!

[weltenwort]

@constancecchen, @chandlerprall thanks for all the feedback! I have changed the RowHeightUtils instantiation to happen inside of a useState initializer, removed the bare usage of forceRender and added tests for useLatest. Let me know what you think.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6050/

[cee-chen]

Fantastic! Thank you so much for taking the time to explain how all these changes worked, it really helped a lot!

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6050/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6050/