[ENDPOINT][TECH DEBT/DISCUSSION] Decide on how to proceed or not with snapshot testing

[efreeti]

In trusted apps for most components I've went with snapshot testing to save time on writing tests and to keep tests maintainable. This triggered mixed opinions on whether it is useful and we should do it. In addition to that to reduce the snapshot size as per remark I've tried to use shallow rendering when ever possible. Shallow rendering is only supported by Enzyme while full rendering of components that use Redux is not possible with Enzyme and forces me to use react-testlib.

This is an issue to prepare points for discussion and have discussion on whether it's a viable option to proceed with.

[elasticmachine]

Pinging @elastic/endpoint-management (Team:Endpoint Management)

[paul-tavares]

Personally, I don't think I will use snapshots much going forward, but don't have an issue with the team using them.

I think the following discussions sums up allot of the feedback I have seen around snapshots (slack):

Frank Hassanabad 1 month ago fwiw, in the past on security_solutions we had guidelines in place where if we did snap shot testing we just used "shallow" to reduce the noise and failures from people updating CSS. However, we noticed that changes were made to Kibana here: https://github.com/elastic/kibana/blob/master/.gitattributes Which causes auto-collapsing of snapshots during diffs and code reviews. This made it very easy for us to: Check in large snapshots without anyone noticing during code reviews. People rarely checked snapshots for diffs during code reviews. Every time EUI updated we were broke or they had to update our snapshots and no one would really review the diffs since the .gitattributes keep them all collapsed by default. So later we decided to forgo most of the snapshot tests as not really useful. Looking at the trusted apps I can see some are already 5k file check-ins... So up to you if you want to keep them (maybe you have better luck than us?). Others have suggested for us to use "inline snapshots" if we want to keep some of them and avoid the collapsing but most people I know have moved away or don't bother with them much on security_solutions (earlier team members at least) I found them useful before collapsing but with the collapsing rule for diffs I don't find them useful anymore and don't review them anymore even if I see them. Btw, from them I found the misspelling in trusted apps of the word "Internal" here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.test.tsx#L65 (edited) .gitattributes https://github.com/elastic/kibana|elastic/kibanaelastic/kibana | Added by GitHub x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.test.tsx:65 https://github.com/elastic/kibana|elastic/kibanaelastic/kibana | Added by GitHub :thu: 1

Paul Tavares 1 month ago Thanks @frank.hassanabad . Great feedback. Yeah, I do like the diff in snapshots and always take a close look at them before commuting (IDE makes easy to review it) and agree that the collapse in github is not helpful in understanding what change (I’m guilty of mostly skipping those files as well during code review :blobstare:) Will certainly reevaluate the use of snapshots for React components in trusted apps (for other snapshots we normally been using online snapshots or just toEqual()). We don’t have many now, so maybe removing them in favor of specific checks make more sense - especially since we use a component library (Eui) that is already well tested. Appreciate your feedback and insight into the paths siem has crossed. Cc/ @bohdan.tsymbala

Frank Hassanabad 1 month ago The inline ones were a good idea as they would put them in your tests directly and really force you to make the snapshots incredibly small when other solutions people mentioned it to us as having good work flows around them. However, the security team kind of became split where some people just didn't think they were of value compared to specific checks, but I did like that idea, I just never got around to it and instead opted for specific hand tests in particular areas as well because I knew those were going to stick around and be looked at if a test failed by another dev. I started taking the trade off of less friction with the tests but more durability and more time to write with the specific unit tests. Either way, :shamrock: , good luck, I always think it's good for people to experiment or revisit things as maybe results become different or something that was an issue is no longer one these days.