-
Notifications
You must be signed in to change notification settings - Fork 668
Update Dynamo samples from xml to json #8657
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
Conversation
|
@alfarok thanks for this - how are the test changes going? |
|
@mjkkirschner These are the failing tests after converting samples to JSON |
| Assert.AreEqual(ws.RunSettings.RunType, RunType.Manual); | ||
| Assert.IsFalse(ws.HasRunWithoutCrash); | ||
| Assert.IsTrue(ws.HasRunWithoutCrash); | ||
| } |
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.
Workspace is no longer exhibiting a crash during this test?
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.
@alfarok I think you may be misinterpreting that - I believe the idea was that this sample file used to have ws.HasRunWithoutCrash set to false, but now it's changed? Can you check that?
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.
@mjkkirschner Ah so if this graph was set to run in manual it would not have executed therefore HasRunWithoutCrash is set to false because the graph hasn't run but hasn't actually crashed.
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.
@QilongTang @mjkkirschner This test is failing because of this issue we original saw when we ran all the tests against JSON
| { | ||
| var examplePath = Path.Combine(SampleDirectory, @"en-US\Basics\Basics_Basic01.dyn"); | ||
| ViewModel.Model.OpenFileFromPath(examplePath); | ||
| ViewModel.OpenCommand.Execute(examplePath); |
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.
The view block was coming back empty using the previous opening strategy causing a series of tests to fail
|
@mjkkirschner @QilongTang I am also still having trouble getting this test to pass. Check out the comments, seems like there were some issues in the past but it fails locally as well. |
|
^The example graph opens and executes normally when opened manually but all the nodes are returning null when run by the test |
|
@alfarok try adding the correct libraries to the preload libraries method, it looks like you would need DSOffice and the file selection nodes. |
|
| } | ||
|
|
||
| [Test] | ||
| [Test, Ignore] |
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.
After discovering hasRunWithoutCrash is defaulting to true and discussing issues found in QNTM-2839 with @QilongTang we have to decided to Ignore this test until we are able to make the necessary changes to how our serialization tests operate. I have noted the issue in the code and added a note to QNTM-2839 about restoring this test when the required modifications are made.
|
LGTM |
Cherry-pick #8657: Update Dynamo samples from XML to JSON
Purpose
QNTM-3304
This PR updates the Dynamo samples that are accessible within the Dynamo menu from xml to json to support the file format change from 1.3 to 2.0. The samples have also been tested against their original result to verify they all run and return equivalent results.
Testing
Original run of the self-serve CI showed several test failures that relied on the sample files modified in this PR from XML to JSON. These have all been addressed and 1 test till be ignored due to an existing task outlined in QNTM-2839
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner