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.

Sign up for GitHub

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

Jump to bottom

DYN-6607 - Input/Output Node - part 1 FIX #15037

Merged
merged 3 commits into from
Mar 22, 2024
Merged

DYN-6607 - Input/Output Node - part 1 FIX #15037

merged 3 commits into from
Mar 22, 2024

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Mar 20, 2024

Purpose

Fixes a number of failing tests blocking master-15.
Ignored tests are already fixed in the follow-up PR #15022 (probably a mix-up when cherry-picking).

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.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

  • fixes a number of tests causing
  • some tests are already fixed due to upcoming changes in a follow-up PR - these were temporarily ignored

Reviewers

@QilongTang
@mjkkirschner

FYIs

@saintentropy
@twastvedt

@dnenov
definedata hotfix
ecaa876 
- fixes a number of tests causing
- some tests are already fixed due to upcoming changes in a follow-up PR - these were temporarily ignored
@QilongTang
Copy link
Contributor

I cant find the pull request validation job for this PR.. So I started https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1415/ for you

@QilongTang
Copy link
Contributor

One regression from the job: Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings

@QilongTang QilongTang added this to the 3.1 milestone Mar 21, 2024
@dnenov
Merge remote-tracking branch 'upstream/master' into definedata-master…
ba3aa2c 
…-fix
@dnenov
TestImportCopySettings fix
6811454 
- fixed TestImportCopySettings by adding the new Preference property to the test settings file
@dnenov
Copy link
Collaborator Author

dnenov commented Mar 22, 2024

One regression from the job: Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings

Thank you, @QilongTang ! I don't know how this went in - it was a new property added to the Preferences - TemplateFilePath. I added that to the settings test file fixing the test.

@dnenov dnenov closed this Mar 22, 2024
@dnenov dnenov reopened this Mar 22, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 22, 2024 •
edited
Loading

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

QilongTang
QilongTang reviewed Mar 22, 2024
View reviewed changes
test/settings/DynamoSettings-NewSettings.xml
@@ -40,6 +40,7 @@
<BackupFiles>
<string>C:\Users\user\Documents\dynamofile.dyn</string>
</BackupFiles>
<TemplateFilePath>C:\ProgramData\Dynamo\Dynamo Core\templates\en-US</TemplateFilePath>
Copy link
Contributor

Choose a reason for hiding this comment

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

@zeusongit It looks like now this property is serialized always? Although the value could be empty. Please confirm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @QilongTang - I am not the author of the preference property, I assume it's a new addition.

Dynamo/src/DynamoCore/Configuration/PreferenceSettings.cs

Line 949 in 97e2d46

TemplateFilePath = string.Empty;

The test picks it up as the number of default preferences coming from PreferenceSettings.cs was different then the one specified in the test file. I just added the entry in the test file to account for the new property and fix the test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if this answers the question, but I am not sure if the property should or shouldn't be serialized.

@QilongTang QilongTang merged commit 97e2d46 into DynamoDS:master Mar 22, 2024
32 checks passed
twastvedt added a commit that referenced this pull request Apr 10, 2024
@twastvedt
Revert "DYN-6607 - Input/Output Node - part 1 FIX (#15037)"
4838e90 
This reverts commit 97e2d46.

# Conflicts:
#	test/Tools/NodeDocumentationMarkdownGeneratorTests/MarkdownGeneratorCommandTests.cs
@twastvedt twastvedt mentioned this pull request Apr 10, 2024
Merged
9 tasks
QilongTang pushed a commit that referenced this pull request Apr 15, 2024
@twastvedt
DYN-4654 Revert IO node development for 3.1 (#15110)
83e388a 
* Revert "DYN-6607 - Input/Output Node - part 1 FIX (#15037)"

This reverts commit 97e2d46.

# Conflicts:
#	test/Tools/NodeDocumentationMarkdownGeneratorTests/MarkdownGeneratorCommandTests.cs

* Revert "DYN-6607 - Input/Output Node - part 1 (#14987)"

This reverts commit c90f105.
@twastvedt twastvedt mentioned this pull request Apr 15, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
2 participants
@dnenov @QilongTang