Skip to content
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

T1.3 save python2 backup file #10928

Merged
merged 58 commits into from
Aug 4, 2020

Conversation

SHKnudsen
Copy link
Contributor

@SHKnudsen SHKnudsen commented Jul 23, 2020

Purpose

This PR adds functionality to the MigrationAssistant which will save a complete backup of the dyn as {dynName}.Python2.dyn. This will only happen the first time the migration assistant is used to update code. As long as there is a {dynName}.Python2.dyn file in the backup folder no additional python 2 backups will be saved.

T1 3-SavePython2Backup

Declarations

Check these if you believe they are true

  • The codebase 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

@mmisol

FYIs

@mjkkirschner
@QilongTang
@radumg

* add ScriptMigrator

* Add visual difference viewer

* Update PythonMigrationViewExtension.cs

* Add tooltip description to migration assistant

* updates

* comment updates

* comment updates
* add ScriptMigrator

* Add visual difference viewer

* Added methods for updating the ScriptEditors with the migrated code

* Update PythonMigrationViewExtension.cs

* Add tooltip description to migration assistant

* updates

* comment updates

* comment updates

* Update ScriptEditorWindow.xaml.cs

* Update PythonMigrationViewExtension.csproj

* Update PythonMigrationViewExtension.csproj

* comment updates

* Revert "comment updates"

This reverts commit 2d2332a.

* comment updates

* Update PythonMigrationAssistantViewModel.cs

* comment updates 2
* add ScriptMigrator

* Add visual difference viewer

* Added methods for updating the ScriptEditors with the migrated code

* Update PythonMigrationViewExtension.cs

* add mechanisme to save Python 2 backup

* Add tooltip description to migration assistant

* updates

* comment updates

* comment updates

* Update ScriptEditorWindow.xaml.cs

* Update PythonMigrationViewExtension.csproj

* Update PythonMigrationViewExtension.csproj

* comment updates

* Revert "comment updates"

This reverts commit 2d2332a.

* comment updates

* Update PythonMigrationAssistantViewModel.cs

* comment updates 2

* small clean ups

* Update PythonMigrationAssistantViewModel.cs
@radumg
Copy link
Collaborator

radumg commented Jul 29, 2020

something along the lines of opening a file, migrating a node and checking that the backup is where it should be, would be a good test to have I guess. Thoughts @SHKnudsen @mjkkirschner ?

This PR's ready now in theory, just this last comment to address if we still want to - we could add this now, but one question i would have is around the fact the migration now produces a MessageBox to warn user that a backup file has been created.

Would this interfere with the test you think (before i delve into it) @mmisol @mjkkirschner ?

@mmisol
Copy link
Collaborator

mmisol commented Jul 29, 2020

@radumg I'm afraid opening a MessageBox could be a problem, but maybe there is a way to close it from code? I know you guys have created some tests in the past that deal with windows, like for the Python warnings. Not sure if something similar can be done.
You might be also be able to use Moq in your test to override the part of the class that shows the dialog, and do an assertion instead.
On a related track, if MessageBox is not our final choice, we might decide to leave this for later, and maybe the test will be simpler. For instance, if we chose to use the Dynamo notifications that appear on the top right instead of the MessageBox, then the problem would no longer exist. Copying @Amoursol to get his opinion regarding this last part.

@radumg
Copy link
Collaborator

radumg commented Jul 30, 2020

I know you guys have created some tests in the past that deal with windows, like for the Python warnings. Not sure if something similar can be done.

Think those were easier since we could get control of that as Window.

You might be also be able to use Moq in your test to override the part of the class that shows the dialog, and do an assertion instead.

Those methods are private for now and non-virtual, not sure exposing them more just for the test would be great?

On a related track, if MessageBox is not our final choice, we might decide to leave this for later, and maybe the test will be simpler. For instance, if we chose to use the Dynamo notifications that appear on the top right instead of the MessageBox, then the problem would no longer exist. Copying @Amoursol to get his opinion regarding this last part.

Defo, happy to come back to this one later on.

@radumg radumg added the Python label Jul 30, 2020
@mmisol
Copy link
Collaborator

mmisol commented Jul 30, 2020

Those methods are private for now and non-virtual, not sure exposing them more just for the test would be great?

You can do that with internal virtual methods. Here is an example: https://github.com/DynamoDS/Dynamo/blob/master/src/VisualizationTests/Preview3DMemoryOutageTest.cs#L35.
For that to work you need to use [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")], so that Moq can access the internal members in that assembly.

@Amoursol
Copy link
Contributor

To confirm @mmisol @radumg - Creating a Messagebox that tells the user we are saving a backup of their Python2 graph causes issues with testing?

@mjkkirschner
Copy link
Member

for many message boxes that are modal we sometimes use isTestMode as a way to turn these off during testing.

@mjkkirschner
Copy link
Member

Screen Shot 2020-07-30 at 12 36 30 PM

7 failing tests.

@mmisol
Copy link
Collaborator

mmisol commented Jul 31, 2020

@Amoursol Yes, but there are workarounds to not show them during the tests, which I think we could apply in the context of this PR. @radumg If you can please take a look at it, as well as the build failures and Mike's last comments. Thanks!

@radumg
Copy link
Collaborator

radumg commented Jul 31, 2020

When we fix the above, also move the ? button to be next to 2->3 one :
#10930 (comment)

@SHKnudsen
Copy link
Contributor Author

@mmisol fixed all of @mjkkirschner comments and added a test to check that the backup function works as expected. Also moved the "More Information" button on the ScriptEditor window so it now sits next to the 2to3 button.
T1 3-MoveMoreInfoButton

Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing @SHKnudsen . Your changes look good to me.

I'm not sure why an automatic build on self server was not triggered but I triggered one manually and everything passed:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1195/

@mjkkirschner
Copy link
Member

let's wait for the final test run, looks good though.

@mjkkirschner mjkkirschner merged commit dc87640 into DynamoDS:master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants