Skip to content

Conversation

@alfarok
Copy link
Contributor

@alfarok alfarok commented Mar 15, 2018

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

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner

@mjkkirschner
Copy link
Member

@alfarok thanks for this - how are the test changes going?

@alfarok
Copy link
Contributor Author

alfarok commented Mar 16, 2018

@mjkkirschner These are the failing tests after converting samples to JSON
ManipulatingWorkspaceDoesNotAffectZoom
OpeningWorkspaceSetsPosition
OpeningWorkspaceSetsZoom
OpeningWorkspaceWithManualRunState
ImportExport_Excel_to_Dynamo

Assert.AreEqual(ws.RunSettings.RunType, RunType.Manual);
Assert.IsFalse(ws.HasRunWithoutCrash);
Assert.IsTrue(ws.HasRunWithoutCrash);
}
Copy link
Contributor Author

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?

Copy link
Member

@mjkkirschner mjkkirschner Mar 16, 2018

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@alfarok alfarok Mar 19, 2018

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);
Copy link
Contributor Author

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

@alfarok
Copy link
Contributor Author

alfarok commented Mar 16, 2018

@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.

@alfarok
Copy link
Contributor Author

alfarok commented Mar 16, 2018

^The example graph opens and executes normally when opened manually but all the nodes are returning null when run by the test

@mjkkirschner
Copy link
Member

@alfarok try adding the correct libraries to the preload libraries method, it looks like you would need DSOffice and the file selection nodes.

@mjkkirschner
Copy link
Member

libraries.Add("VMDataBridge.dll"); // Required for Watch node.

}

[Test]
[Test, Ignore]
Copy link
Contributor Author

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.

@alfarok alfarok changed the title Update Dynamo samples from xml to json [PTAL] Update Dynamo samples from xml to json Mar 19, 2018
@QilongTang
Copy link
Contributor

LGTM

@alfarok alfarok merged commit f08eff0 into DynamoDS:master Mar 19, 2018
alfarok added a commit that referenced this pull request Mar 19, 2018
Cherry-pick #8657: Update Dynamo samples from XML to JSON
@alfarok alfarok changed the title [PTAL] Update Dynamo samples from xml to json Update Dynamo samples from xml to json Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants