Assertion Template Enhancements #314
Conversation
|
Don't forget to update https://github.com/dojo/framework/blob/master/src/testing/README.md |
matt-gadd
added
area: testing
aciccarello
left a comment
aciccarello
left a comment
There was a problem hiding this comment.
Looking forward to this change.I know this is a WIP PR but I wanted to leave a couple notes on things I noticed when checking this out.
The CLI will need to have a codemod to rename the old replace function as well.
src/testing/assertionTemplate.ts
Outdated
| const parent = (node as any).parent; | ||
| const children = [...parent.children]; | ||
| children.splice(children.indexOf(node), 1); | ||
| parent.children = [node, ...children]; |
There was a problem hiding this comment.
Wouldn't this always put the replacement node at the beginning of the children? You should be able to pass the replacement with .splice().
There was a problem hiding this comment.
thanks was being silly here from the copy pasta from remove
| const parsedExpected = formatDNodes(expected); | ||
| const [decoratedActual, decoratedExpected] = decorate(actual, expected); | ||
| const parsedActual = formatDNodes(Array.isArray(actual) ? decoratedActual : decoratedActual[0]); | ||
| const parsedExpected = formatDNodes(Array.isArray(expected) ? decoratedExpected : decoratedExpected[0]); |
There was a problem hiding this comment.
Can you explain what is going on with the isArray check? Shouldn't this be changed to the following?
| const parsedExpected = formatDNodes(Array.isArray(expected) ? decoratedExpected : decoratedExpected[0]); | |
| const parsedExpected = formatDNodes(Array.isArray(decoratedExpected) ? decoratedExpected[0] : decoratedExpected); |
There was a problem hiding this comment.
@aciccarello the decorator function always turns nodes into an array (if it isn't already) and returns an array. the assertRender function and formatDNodes take an individual node or an array. so all this does is use the first node if the original nodes (expected and actual) were a singular node. So no the suggested change is incorrect. I might change the decorate function to align with formatDNodes to avoid this slightly confusing conversion though.
src/testing/support/assertRender.ts
Outdated
| let expectedNode = expected[i]; | ||
|
|
||
| if (isWNode(expectedNode)) { | ||
| [actualNode, expectedNode] = assertionWidgets.reduce( |
There was a problem hiding this comment.
this part is somewhat confusing to me. I can follow what it is doing, but it isn't clear to me why it i doing what it is doing. If you are changing the actualNode and expectedNode references, does it really make sense to use reduce here? I think a forEach might end up being simpler.
There was a problem hiding this comment.
I agree, i just removed the loop completely in the end as the indirection wasn't worth it.
…es and getProperties
matt-gadd
force-pushed
the
assertion-template-enhancements
branch
from
a40e640 to
b9a3bcc
Compare
matt-gadd
changed the title
|
@matt-gadd is there anything I can do to help with this? |
|
superseded by #412 |
3 participants
Type: feature
The following has been addressed in the PR:
prettieras per the readme code style guidelinesDescription:
TODO: Fill this out explaining enhancements.
API's changed:
(this #247 (comment) came back to haunt me)
API's added:
New Assertion Template primitive: