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

Support Net6 target for DynamoCoreTests.dll (WIP for feedback) #13766

Merged
merged 54 commits into from
Mar 6, 2023

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Feb 21, 2023

Purpose

This PR starts to move tests in DynamoCoreTests to .NET6.

It's still a WIP - but I think it's ready for feedback.

does the following:

  • even though tests now build on linux- testing on github action is still blocked - I filed dotnet test on linux fails to find tests microsoft/vstest#4330
  • removes duplicate -g for geometryPath / geometry mode in CLI and moves to Geometry only
  • adds DisableAnalytics to unify tests that use CLI.
  • adds DynamoCoreTests,TestServices,DSOffice and FFITarget .csprojs to the DynamoCoreNet6.sln.
  • unifies on a new version of Moq which can be referenced from NET6 -
    • ⚠️ this brings in new dependencies to the bin folder - I have tried to filter them out during the install.sln so they are not delivered These are now copied to the test/test_dependencies folder and resolvers added to look here during testing.
    • Existing issue - I have noticed that some test binaries are already being included since at least 2.8 - thats as far back as I went - (FFITarget,Moq, Nunit.Framework) - it appears that exclusions need to be added to the robo copy step in dynamoinstall.csproj - hopefully we come up with a more holistic solution before the next GL. https://jira.autodesk.com/browse/DYN-5673
    • updates use of moq to apis in new version.
  • make DSOffice.csproj conditional so it does not reference or build exel interop nodes for net6 (open xml is built)
  • conditionally updates DynamoCoreTests.csproj to Nunit3 for NET6
    • ⚠️ - serialization tests fail when comparing dictionaries now - I've created a test only comparison func that seems to work using sequence equals on keys and vals - only for DynamoDictionaries.
    • use #ifdefs to move to new NUnit3 asserts and attributes.
    • marks some tests as Category("FailureNET6")] - some of these can be fixed with more complex refactoring so we can file a followup.
  • DynamoCoreTests had some transitive WPF references - attempted to refactor the easy ones by moving tests to DynamoCoreWFPtests.csproj.
  • AssemblyResolve behavior has changed between .net48 and .net6 - runtime better handles resolvers that throw exceptions. (this required test changes)
  • unify on Microsoft.NET.Test.Sdk 17 between both solutions.
  • skip a few migration tests that check excel and legacy unit nodes when targeting .net6.
  • Moves PythonMigration tests to DynamoPythonTest project
  • TypeForwards TestExtensions from SystemTestServices to TestServices so it can be used in DynamoCoreTests without transitive WPF dep.

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

Release Notes

Support Net6 target for DynamoCoreTests.dll

Reviewers

test/DynamoCoreTests/NodeMigrationTests.cs Outdated Show resolved Hide resolved
test/DynamoCoreTests/UnitsOfMeasureTests.cs Show resolved Hide resolved
@@ -293,7 +293,8 @@ public static void CompareWorkspaceModels(serializationTestUtils.WorkspaceCompar
// When values are geometry, sometimes the creation
// of the string representation for forming this message
// fails.
Assert.AreEqual(valueA, valueB,

Assert.That(valueA, Is.EqualTo(valueB).Using<Dictionary>(DynamoDictionaryEquality),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor

Choose a reason for hiding this comment

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

how are we running tests for a wpf project ?
Or what is this change about ?
I see there is also a new Setup.cs file ? why ?

Copy link
Member Author

@mjkkirschner mjkkirschner Mar 6, 2023

Choose a reason for hiding this comment

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

because now Moq (v 4.18 - even on net48) has new dependencies which I only copy to the test/test_dependencies folder. So we need to setup a setupfixture resolve handler to find them.

Copy link
Contributor

Choose a reason for hiding this comment

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

It just seemed to me that most of the Visualization tests are derived from SystemTestBase which already uses an AssemblyResolver which we could use

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but not all of them - then we'd have two places to remember to look for this resolve to occur.

<Copy SourceFiles="$(PkgSystem_Buffers)\lib\netstandard2.0\System.Buffers.dll" DestinationFolder="$(TestDependenciesPath)" />
</Target>

<Target Name="MoveSomeTestDepsOutOfBin_net48" AfterTargets="Build" Condition=" '$(TargetFramework)' == 'net48' ">
Copy link
Contributor

Choose a reason for hiding this comment

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

so we are moving stuff out even on net48 ? That will not break Revit installers ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no because these are new dependencies of Moq - and I put Moq back in the bin folder as the last step:

        <Copy SourceFiles="$(PkgMoq)\lib\netstandard2.0\Moq.dll" DestinationFolder="$(OutputPath)" />

@@ -0,0 +1,43 @@
using Dynamo.Utilities;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is a new Setup.cs needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

same reason as above, this assembly uses moq.

Copy link
Contributor

@pinzart90 pinzart90 left a comment

Choose a reason for hiding this comment

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

A couple of questions, then LGTM

@mjkkirschner
Copy link
Member Author

the test failure appears to have come from here:
#13794
merging.

@mjkkirschner mjkkirschner merged commit 1088c1e into DynamoDS:master Mar 6, 2023
@mjkkirschner mjkkirschner deleted the net60tests branch March 6, 2023 21:44
sm6srw pushed a commit to sm6srw/Dynamo that referenced this pull request Mar 29, 2023
…oDS#13766)

* add new action

* try general var syntax

* update

* dont use regen in core

* try also not generating resource binaries for string resources

* pull actual code changes

* linux paths

* test command

* add single test project to sln

* add new nuspec

* add post script

* revert adding test proj
build portable pdbs

* build portable in release
update nuspec

* remove publish script

* remove test from github action

* review comments

* by hook or by crook we'll reuse existing test base classes.

* this works

* both solutions build at this point
unclear if tests pass

* try running some linux tests

* build current branch from fork

* updates

* updates

* update

* ugh fix bug

* fix test

* fix more tests

* fix more tests

* fix mono build

* try cat = unittests

* linux not finding any tests

* Update dynamoNet6.0_linux_build.yml

* dont run tests

* dont include castle core in build
upgrade moq

* remove net60smoketests
unify on 17 version of ms.net.test.sdk

* update python and fix refs

* remove duplicate ms.net.test.sdk ref

* introduce junit logger
add back ms test sdk - its needed in bin folder.

* use custom dictionary comparison function in nunit

* compare keys and values when encountering dictionary - toString was not precise enough.

* fix test
mark test failure

* introduce a failure

* update dscpython with split

* fix build for nunit2

* remove failure

* copy moqs deps to test deps directory

* fix

* find moq for tests that were failing

* review comments

* review comments

* mark failure
sm6srw pushed a commit that referenced this pull request Apr 5, 2023
* add new action

* try general var syntax

* update

* dont use regen in core

* try also not generating resource binaries for string resources

* pull actual code changes

* linux paths

* test command

* add single test project to sln

* add new nuspec

* add post script

* revert adding test proj
build portable pdbs

* build portable in release
update nuspec

* remove publish script

* remove test from github action

* review comments

* by hook or by crook we'll reuse existing test base classes.

* this works

* both solutions build at this point
unclear if tests pass

* try running some linux tests

* build current branch from fork

* updates

* updates

* update

* ugh fix bug

* fix test

* fix more tests

* fix more tests

* fix mono build

* try cat = unittests

* linux not finding any tests

* Update dynamoNet6.0_linux_build.yml

* dont run tests

* dont include castle core in build
upgrade moq

* remove net60smoketests
unify on 17 version of ms.net.test.sdk

* update python and fix refs

* remove duplicate ms.net.test.sdk ref

* introduce junit logger
add back ms test sdk - its needed in bin folder.

* use custom dictionary comparison function in nunit

* compare keys and values when encountering dictionary - toString was not precise enough.

* fix test
mark test failure

* introduce a failure

* update dscpython with split

* fix build for nunit2

* remove failure

* copy moqs deps to test deps directory

* fix

* find moq for tests that were failing

* review comments

* review comments

* mark failure
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.

3 participants