Remaining watch account ui tests #446
Conversation
| }) | ||
| // store details of the displayed addresses for later comparison | ||
| multisigPage.accountHeader().within(() => { | ||
| accountDisplay.addressLabel().should('be.visible').invoke('text').as('headerPureAddress') |
There was a problem hiding this comment.
I've been searching for a way to achieve the same, in an easier to read way. I've always found aliases pretty convoluted, with the @ etc.
This is nice and generic, but can't we just compare it to what we expect? After all this is all just testing that the pure (and down the file the multisig) address is the same isn't it? It would make this test easier to read and maintain IMHO.
There was a problem hiding this comment.
What I preferred about using the aliases is that when we were inspecting the UI and watching via the pure we were dynamically grabbing and storing whatever data was displayed (without caring, just knowing this is the view when a pure is watched) and then only asserting when we watch via the multisig that the displayed details are the same as what we saw previously.
I've updated now to compare using the static values as requested but this means we are now first asserting on the display when watching a pure key (already covered in other tests) and then again the displayed data when watching a multisig key, a trivial difference that probably makes no change in execution time but it's less of a well-written test in my opinion. We break test writing conventions of only asserting after the point of change (but admittedly we do this all the time especially with should('be.visible') checks because we have to 😄).
Ultimately if it reads better I guess that matters more but I wanted to explain my view. I still think cypress alias will have their place when checking dynamic data but perhaps it makes less sense when values are static like this (and I do agree the @ syntax is a little odd!).
There was a problem hiding this comment.
Maybe with my PR using the exact same code, this reads even better, this is how I was thinking about this test originally. Sure it's a bit dumb and we're checking twice the same things (now not repeated, but in a function) but how we arrived at this state is different, and that's what matters.
There was a problem hiding this comment.
Yep this reads well, thanks :)
Co-authored-by: Thibaut Sardan <[email protected]>
There was a problem hiding this comment.
Thanks for the explanation, I totally get you. I def think it makes sense in some places, but it really felt too much here. BTW I created #450 that makes it so that we don't repeat the exact same test twice. if you're ok with it, please merge it first, then you can merge this one.
closes #378
A couple of new tests to cover the last remaining watch account scenarios on the issue.