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

Upgrade to net8-windows #11

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Upgrade to net8-windows #11

wants to merge 7 commits into from

Conversation

rwmcintosh
Copy link
Member

No description provided.

rwmcintosh
rwmcintosh commented Mar 21, 2024
View reviewed changes
SolutionInfo.cs
[assembly: AssemblyCompany("Open Systems Pharmacology Community")]
[assembly: AssemblyProduct("OSPSuite.DataBinding")]
[assembly: AssemblyCopyright("Copyright � 2002-2019 - Open Systems Pharmacology Community")]
[assembly: ComVisible(false)]
[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
[assembly: InternalsVisibleTo("OSPSuite.DataBinding.Tests")]
[assembly: SupportedOSPlatform("windows")]
Copy link
Member Author

Choose a reason for hiding this comment

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

We need this to tell VS that all assemblies are targeting windows only, otherwise there is a lot of warnings generated.

rwmcintosh
rwmcintosh commented Mar 21, 2024
View reviewed changes
src/OSPSuite.DataBinding/OSPSuite.DataBinding.csproj Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

the csproj files are upgraded by right click, then select Upgrade. There's a few confirmations etc, but it's automatic. You just have to pick the target.

Then you have to go back and put -windows by hand.

@rwmcintosh
Copy link
Member Author

I was able to run the starter ok many of the tests pass as well.

There is a test named should_use_the_validation_engine_to_validate_the_element that fails with stack overflow during this call to ScreenBinder.ClearError

image

Welcome any help you can add, although, we could get away without upgrading these projects that are Windows specific for now.

But, since we're doing it, let's really do it... you know?

@rwmcintosh
Copy link
Member Author

Appveyor is also not picking up the tests. Not sure why

rwmcintosh
rwmcintosh commented Mar 21, 2024
View reviewed changes
tests/OSPSuite.DataBinding.Tests/ElementBinderValidatorSpecs.cs
@@ -22,7 +23,8 @@ protected override void Context()
A.CallTo(() => _elementToValidate.Source).Returns(_source);
A.CallTo(() => _elementToValidate.PropertyName).Returns("FirstName");
A.CallTo(() => _elementToValidate.GetValueFromControl()).Returns("Toto");
_elementToValidate.ParentBinder = _screenBinder;
A.CallTo(() => _elementToValidate.ParentBinder).Returns(_screenBinder);
A.CallTo(() => _elementToValidate.Control).Returns(new Label());
Copy link
Member Author

Choose a reason for hiding this comment

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

That was the line needed to eliminate stack overflow exception. Looks like it was related to tests, not to the code itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Appveyor still not running any tests though

@rwmcintosh rwmcintosh changed the base branch from master to main March 21, 2024 17:17
@msevestre msevestre deleted the branch main March 21, 2024 17:42
@msevestre msevestre closed this Mar 21, 2024
@msevestre msevestre reopened this Mar 21, 2024
@rwmcintosh
Copy link
Member Author

rwmcintosh commented Mar 21, 2024 •
edited

The next hurdle is this:

nunit won't run these tests because by default nunit3-console is started as a netcore app and cannot access the windows runtime. You can patch the runtimeconfig.json file to make it work
make it:

{
  "runtimeOptions": {
    "tfm": "net8.0",
    "rollForward": "Major",
    "framework": {
      "name": "Microsoft.WindowsDesktop.App",
      "version": "8.0.0"
    },
    "configProperties": {
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

instead of:

{
  "runtimeOptions": {
    "tfm": "net8.0",
    "rollForward": "Major",
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "8.0.0"
    },
    "configProperties": {
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

Or there are ways of adding multiple frameworks.

At least, this is the reason the console runner would not work for me locally. AppVeyor isn't giving too many hints.

@rwmcintosh rwmcintosh changed the title For discussion - upgrade to net8-windows Upgrade to net8-windows Mar 21, 2024
Yuri05
Yuri05 reviewed Mar 22, 2024
View reviewed changes
SolutionInfo.cs Outdated
@@ -1,10 +1,12 @@
using System.Runtime.CompilerServices;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
[assembly: AssemblyCompany("Open Systems Pharmacology Community")]
[assembly: AssemblyProduct("OSPSuite.DataBinding")]
[assembly: AssemblyCopyright("Copyright � 2002-2019 - Open Systems Pharmacology Community")]
Copy link
Member

Choose a reason for hiding this comment

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

Minor issue: could change the copyright to ... 2002-2024...

@Yuri05
Copy link
Member

Yuri05 commented Mar 22, 2024

At least, this is the reason the console runner would not work for me locally. AppVeyor isn't giving too many hints.

Can you temporarily add

build:
  verbosity: detailed

to the appveyor.yml? (compare https://www.appveyor.com/docs/appveyor-yml/ )
It seems that we have the same problems (no tests executed) also for other Libs, e.g. here Open-Systems-Pharmacology/OSPSuite.Serializer#5)

@rwmcintosh
Copy link
Member Author

At least, this is the reason the console runner would not work for me locally. AppVeyor isn't giving too many hints.

Can you temporarily add

build:
  verbosity: detailed

to the appveyor.yml? (compare https://www.appveyor.com/docs/appveyor-yml/ ) It seems that we have the same problems (no tests executed) also for other Libs, e.g. here Open-Systems-Pharmacology/OSPSuite.Serializer#5)

no luck with any additional details

Discovering tests...OK
Build success

@Yuri05
Copy link
Member

Yuri05 commented Mar 22, 2024

Can you try to use the test script with the explicit call to dotnet test, e.g. like here:
https://github.com/Open-Systems-Pharmacology/OSPSuite.SimModel/blob/c873aea97e28aee9c0f930872664c7143f31c9e8/appveyor.yml#L69-L70

s. also this discussion https://help.appveyor.com/discussions/problems/9856-appveyor-not-discovering-my-xunit-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuri05 Yuri05 Yuri05 left review comments

@msevestre msevestre Awaiting requested review from msevestre

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@rwmcintosh @Yuri05 @msevestre