-
Notifications
You must be signed in to change notification settings - Fork 635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DYN-1541: Issues with code block function default arguments #9528
Changes from all commits
56dbd73
eec64b9
4362151
75643c7
6a07e6c
38b8461
44a00a8
5c04a06
a21fd6a
4235133
302b9dd
6bf5548
8d4511b
8f9df0f
e91ef30
82373cc
1434ba0
706974c
760ae95
6ffd883
5b1c073
30f81af
ccc23ef
bab2f92
75e430a
0bf25b4
4e7ed90
e64b640
072af66
e4ae2c6
c06d338
396e51d
1e81689
6ad7073
3dfe388
fb22dd2
042ec65
57f2193
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,87 @@ public void TestStatement_ArrayReference() | |
#endif | ||
protected double tolerance = 1e-4; | ||
|
||
// Note: This test fails, even though the individual items pass as separate tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is here to show an issue with the testing system, it can be removed if desired, not sure what the policy is for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the failure mode here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The failure mode is that the second test fails even though if the two tests are run in separate test runs they both pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look at TestFunctionDefaultParameters versus the following TestFunctionSingleDefaultParameter and TestFunctionTwoDefaultParameters you can see that the tests are the same, yet if they are concatenated into one test the test will fail. I did not really look into why, I was just notating that there is potentially an issue with the testing system there that may trip up other people in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm - the difference between running these in a single test and and in multiple tests is that Dynamo is being shut down and new workspaces are being opened in the new tests... that could be a problem with the testing system or it could be a real bug... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when you interact manually with Dynamo you cannot reproduce this failure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To memorialize our recent Zoom conversation, each test passes separately, but putting them both in the same test case fails. Put another way, I can reproduce both issues manually, but in the testing framework having both tests run together fails. I would guess at this point that it is an issue with the testing framework and not Dynamo, but this is just a guess at this point. The main thought was just to get it notated so that other people may avoid some future pain, I'm not sure how much of importance this particular issue has for the everyday user. |
||
// This is preventing putting all of the tests in a single method | ||
//[Test] | ||
//[Category("UnitTests")] | ||
//public void TestFunctionDefaultParameters() | ||
//{ | ||
// var codeBlockNode = CreateCodeBlockNode(); | ||
// var guid = codeBlockNode.GUID.ToString(); | ||
|
||
// UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1){return = x;}test();"); | ||
// AssertPreviewValue(guid, 1); | ||
|
||
// UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2){return = x + y;}test();"); | ||
// AssertPreviewValue(guid, 3); | ||
//} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestFunctionSingleDefaultParameter() | ||
{ | ||
var codeBlockNode = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1){return = x;}test();"); | ||
AssertPreviewValue(codeBlockNode.GUID.ToString(), 1); | ||
} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestFunctionTwoDefaultParameters() | ||
{ | ||
var codeBlockNode = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2){return = x + y;}test();"); | ||
AssertPreviewValue(codeBlockNode.GUID.ToString(), 3); | ||
} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestFunctionThreeDefaultParameters() | ||
{ | ||
var codeBlockNode = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2, z:int= 3){return = x + y + z;}test();"); | ||
AssertPreviewValue(codeBlockNode.GUID.ToString(), 6); | ||
} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestFunctionOneArgumentOneDefaultParameter() | ||
{ | ||
var codeBlockNode = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int, y:int= 2){return = x + y;}test(1);"); | ||
AssertPreviewValue(codeBlockNode.GUID.ToString(), 3); | ||
} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestFunctionMultipleBlocksDefaultParameters() | ||
{ | ||
var codeBlockNode1 = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode1, "def test1(x:int = 1, y:int= 2){return = x + y;}test1();"); | ||
var codeBlockNode2 = CreateCodeBlockNode(); | ||
UpdateCodeBlockNodeContent(codeBlockNode2, "def test2(x, y = 2, z = 3){return = x + y + z;}test2(1);"); | ||
|
||
AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3); | ||
AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6); | ||
} | ||
|
||
// Note: DYN-1684 - This test is expected to fail due to the functions being indeterminate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is here for notating the issue of code blocks with duplicate function names. It is not really part of this issue and can be removed if desired, but I left it in to memorialize the issue for the future. Note that once this duplicate name issue is addressed this test will not be an accurate description of the functionality and will need to be changed or removed. |
||
// We will need to figure out a way to test this once error handling is implemented | ||
//[Test] | ||
//[Category("UnitTests")] | ||
//public void TestIndeterminateFunctionDefaultParameters() | ||
//{ | ||
// // Note that both code blocks contain functions called "test" that are indeterminate | ||
// var codeBlockNode1 = CreateCodeBlockNode(); | ||
// UpdateCodeBlockNodeContent(codeBlockNode1, "def test(x:int = 1, y:int= 2){return = x + y;}test();"); | ||
// var codeBlockNode2 = CreateCodeBlockNode(); | ||
// UpdateCodeBlockNodeContent(codeBlockNode2, "def test(x, y = 2, z = 3){return = x + y + z;}test(1);"); | ||
|
||
// AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3); | ||
// AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6); | ||
//} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestVarRedefinitionInFunctionDef() | ||
|
@@ -224,7 +305,7 @@ public void TestVarRedefinitionInFunctionDef() | |
var guid = "bbf7919d-d578-4b54-90b1-7df8f01483c6"; | ||
var cbn = CurrentDynamoModel.CurrentWorkspace.NodeFromWorkspace<CodeBlockNodeModel>( | ||
Guid.Parse(guid)); | ||
|
||
|
||
Assert.IsNotNull(cbn); | ||
Assert.AreEqual(ElementState.PersistentWarning, cbn.State); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,5 +182,15 @@ public void TestSyntaxHighlightRuleForDigits() | |
string[] expected = new string[] { "-2468.2342E+04", "34534.345345", "23423", "-98.7", "0", "10", "2", "-555" }; | ||
Assert.IsTrue(expected.SequenceEqual(actual)); | ||
} | ||
|
||
[Test, RequiresSTA] | ||
public void TestFunctionMultipleBlocksDefaultParametersDeleteFirst() | ||
{ | ||
RunCommandsFromFile("DeleteCrashCommands.xml", (commandTag) => | ||
{ | ||
// NOTE: Nothing needs to be tested here other than that this test does not crash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this need to be wrapped in an assert does not throw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably true, I will add this |
||
}); | ||
} | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find this style with no braces kind of difficult to read / prone to future errors - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually find the requirement to have the extra braces unnecessary and creates code that is more unreadable (mostly due to adding unnecessary code and code lines), but the bracing wars is a long running issue across multiple companies, I really have no dog in this race. That being said, these changes were made by Aparajit, I can change them however you would like.