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

add extra files in nugets #14774

Merged
merged 13 commits into from
Jan 11, 2024
Merged

add extra files in nugets #14774

merged 13 commits into from
Jan 11, 2024

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Dec 16, 2023

This PR addresses the following issues:
Set DynamoCorePath from systemTestBase

  1. If the test configuration is present, then it will use its values to se the DynamoCorePath
  2. If the test configuration is not present, then it will use the default value of Assembly.GetExecutingAssembly().Location (which should point to the SystemTestBase dll locatioN)

Adds TestServices.dll.config to the Tests nuget (this config is needed to specify the location of the Dynamo runtime folder)
Removed nunit from core nuget (added nunit deps to the test nuget)
Removed Immutable dependency from the core nuget becuse it is now part of dotnet runtime
Removed assembly names from the description (since nuget.org can now explore assemblies of packages)

More explanations on changes to SysteTestBase GetNode method:
nunit wants to resolve public types at the discovery stage. Looks like type constraints are stored with the containing type information. Basically, at the discovery stage, types referenced in "type constraints" must be resolved.
Removing the "NodeModel" type reference in the constraint ("where T : NodeModel") allows discovery to run successfully without having DynamoCore.dll included in the test output directory
Created a follow-up task (DYN-6591)to look into proper API layer for tests

Copy link

github-actions bot commented Dec 16, 2023

":white_check_mark: Bin-Diff Issue Resolved."
(Updated: 2023-12-22-18:49:45)

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

Test: failure. 1 passed, 1 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

@pinzart90 pinzart90 changed the title add extra files in nugets [WIP] add extra files in nugets Dec 18, 2023
@pinzart90
Copy link
Contributor Author

[run-bin-diff-net60-windows] - Files Added/Deleted::1 new file(s) have been added
The TestServices.dll.config file is added to the build output now. This is only useful during testing and to add to our nugets
@mjkkirschner @sm6srw do I need to exclude this from the installer or release builds somehow ?

@pinzart90 pinzart90 changed the title [WIP] add extra files in nugets add extra files in nugets Dec 19, 2023
@mjkkirschner
Copy link
Member

[run-bin-diff-net60-windows] - Files Added/Deleted::1 new file(s) have been added The TestServices.dll.config file is added to the build output now. This is only useful during testing and to add to our nugets @mjkkirschner @sm6srw do I need to exclude this from the installer or release builds somehow ?

I would filter it out if you believe it could have some effect on behavior -
... I think you could add it here:
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoInstall/DynamoInstall.wixproj#L77C110-L77C110

though I hope we replace this installer project eventually with just a single simple script.

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

UI Smoke Tests

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

Copy link

github-actions bot commented Jan 3, 2024

UI Smoke Tests

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

@@ -305,15 +306,15 @@ public void AssertPreviewCount(string guid, int count)
Assert.AreEqual(count, data.GetElements().ToList().Count);
}

public NodeModel GetNode<T>(string guid) where T : NodeModel
protected NodeModel GetNode(string guid)
Copy link
Contributor Author

@pinzart90 pinzart90 Jan 3, 2024

Choose a reason for hiding this comment

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

nunit wants to resolve public types at the discovery stage. Looks like type constraints are stored with the containing type information. Basically, at the discovery stage, types referenced in "type constraints" must be resolved.
Removing the "NodeModel" type reference in the constraint ("where T : NodeModel") allows discovery to run successfully without having all dynamo assemblies included in the test outout directory

Copy link
Member

Choose a reason for hiding this comment

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

so this will yield a runtime failure now if the assembly is not resolved, but at least we'll discover the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will trigger a runtime exception now (instead of compile time) if the T type is not derived from NodeModel.
I marked the method as obsolete so we will be able to replace it with something else that will not cause the same issue

@@ -0,0 +1,9 @@
<Project>
<ItemGroup>
<Content Include="$(MSBuildThisFileDirectory)TestServices.dll.config">
Copy link
Member

@mjkkirschner mjkkirschner Jan 3, 2024

Choose a reason for hiding this comment

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

when does this props file get used? When packing? When unpacking? At build time in the code that references this package?

Copy link
Member

Choose a reason for hiding this comment

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

I am just curious, is there a simpler way to get this config file to be copied - putting it in the lib subfolder folder does not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Props files (from nugets) are consumed by the project that references the nuget when running restore.
This is a pretty standard way to have both assemblies and generic files in a nuget pacakge. There is another way to add generic files (as contentFIles) but those seem to be readonly (and we need write access).
https://learn.microsoft.com/en-us/nuget/concepts/msbuild-props-and-targets#packagereference-projects

8) VMDataBridge.dll
9) DSCPython.dll
10) DynamoPackages.dll
Assemblies required to reference core APIs from Dynamo.
Copy link
Member

Choose a reason for hiding this comment

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

👍

<dependency id="DynamoVisualProgramming.ZeroTouchLibrary" version="$Version$"/>
<dependency id="Newtonsoft.Json" version="13.0.1"/>
Copy link
Member

Choose a reason for hiding this comment

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

do you think this will cause problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, removing it would probably cause build issues for some consumers.
NewtonSoft types still seem to be publicly exposed "near" core Dynamo APIs. I will add a task to mark those as obsolete...or move them somewhere more hidden.

Copy link
Member

@mjkkirschner mjkkirschner Jan 9, 2024

Choose a reason for hiding this comment

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

yeah, it's going to be a big lift to get rid of these types as we require users to use those attributes, perhaps if those attributes are also now facaded by system.text.json we can switch it out without breaking previous builds - but needs to be determined. It was something I was hopeful we could do for 3.0, but no dice.

@mjkkirschner
Copy link
Member

@pinzart90 it looks good, but I don't see the config being excluded from the installer/harvest folder and because the dynamo basepath is now set from the config in the system test base, it may affect the tests, maybe we should exclude it as you suggested above.

Copy link

github-actions bot commented Jan 5, 2024

UI Smoke Tests

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

@pinzart90
Copy link
Contributor Author

@pinzart90 it looks good, but I don't see the config being excluded from the installer/harvest folder and because the dynamo basepath is now set from the config in the system test base, it may affect the tests, maybe we should exclude it as you suggested above.

The config file is now part of test nugets only (not part of the full dynamo runtime). It should not cause any issues with our existing CICD infrastructure.

@@ -305,15 +306,27 @@ public void AssertPreviewCount(string guid, int count)
Assert.AreEqual(count, data.GetElements().ToList().Count);
}

public NodeModel GetNode<T>(string guid) where T : NodeModel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the type constraint "NodeModel" because the nunit discovery process needed to resolve it.
I does not seem to matter if the method is private or public. The type constraint will still need to be resolved along with the containing type "SystemTestBase"

Copy link

github-actions bot commented Jan 8, 2024

UI Smoke Tests

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

/// <param name="guid">The node Guid</param>
/// <returns></returns>
/// <exception cref="Exception"></exception>
[Obsolete("This method will be removed in a future version of Dynamo")]
Copy link
Member

Choose a reason for hiding this comment

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

will it be replaced with some other method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just did not want to overcomplicate de PR now. I can change the text of the obsolete method wen the new one is implemented

Copy link

UI Smoke Tests

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

@pinzart90
Copy link
Contributor Author

@pinzart90 pinzart90 merged commit 321d5ed into master Jan 11, 2024
20 of 21 checks passed
@pinzart90 pinzart90 deleted the fix_samples_tests branch January 11, 2024 14:47
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.

5 participants