Refactoring navigation tests: help

I’d like additional input on this PR. Number one: What is the best way to resolve the Cyprus anti-pattern of assigning return values? Number 2: Regarding const addEventListener = win.EventTarget.prototype.addEventListener; is it actually violating the eslint unbound-method rule? If my hypothesis is wrong, then what is the correct way to write it?

Hello there,

Some things I would do:

  1. Fix the type issue with onBeforeLoad - there is no point passing in a function that takes 4 args, when the caller only expects 1.
  2. Dig into Cypress docs to see what methods already exist to wait for a page to load. The whole workflow seems off to me. The reliance on a global variable throws a red flag. The function’s purpose is just to signal (wait) for the page to load, and to do this does some weird stuff with globals (addEventListener) - this is what the TS error is going on about, because it is dangerous to play with the references the way that code is. This all seems unnecessary.

Actually, thinking about it, I would first try see why the blamer added this in the first place, by removing it entirely, and seeing if the test works without it. We might have needed it with an older version of Cypress, and now do not :man_shrugging:

Hope this helps

Using promises isn’t the perfect replacement but I think I understand at least why the method existed. It was a React state update on an unmounted component.