DYN-1261 Escape StringInputNode value when converting NodeToCode #2
Conversation
StudioLE
changed the title
StudioLE
force-pushed
the
DYN-1261-Escape-NodeToCode-Strings
branch
from
2512513 to
cf8a79a
Compare
StudioLE
changed the base branch from
Internal-Review-RC2.8
to
internal-review-master
StudioLE
changed the title
|
I've added a simple test in 6526482 so figured you might have further comments? @SHKnudsen |
| var nodes = CurrentDynamoModel.CurrentWorkspace.Nodes; | ||
| var engine = CurrentDynamoModel.EngineController; | ||
| var result = engine.ConvertNodesToCode(nodes, nodes); | ||
| Assert.IsNotNull(result.AstNodes); | ||
|
|
||
| var assignment = result.AstNodes.FirstOrDefault(); | ||
| Assert.IsNotNull(assignment); | ||
|
|
||
| var binaryExpr = assignment as BinaryExpressionNode; | ||
| Assert.IsNotNull(binaryExpr); |
There was a problem hiding this comment.
Is all of this actually needed and if it is what are we using it for?
Shouldnt we just test that the nodes preview value is the expected?
There was a problem hiding this comment.
I looked into this further and the other NodeToCode tests are extracting the the BinaryExpressionNode and checking the RightNode in order to check the value of the CodeBlockNode itself. As this seems to be a more explicit way of checking that that NodeToCode has worked as expected I've revised the test to follow this methodology in 3e11fc3.
| .ToList(); | ||
|
|
||
| // Assert | ||
| var expect = new List<string> |
There was a problem hiding this comment.
Nice, makes sense now!
If we are really picky the expect list should probably go into the Arrange section
Purpose
JIRA: DYN-1261
Related Issues:
Ensure that string containing
"and\are correctly escaped when convertingStringInputNodeusing NodeToCode.Behaviour before:
Behaviour after:
Declarations
Check these if you believe they are true
*.resxfilesReviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of