Skip to content

NH-3990 - Upgrade VB test project to VS2017 project structure #670

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

Merged
merged 3 commits into from
Aug 14, 2017

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Aug 6, 2017

This is a continuation of #605.

  • Adds the ability on all test projects to be ran from dotnet test.
  • Accounts for some changes in the VB compiler around CompareString methods being generated from LINQ queries.
  • Also, some test behavior is different on my machine in release mode when build with dotnet build, so that is addressed. Specifically, IEnlistmentNotification.Prepare doesn't seem to be guaranteed to be called from a different thread like it asserts.

methodInfo.DeclaringType.FullName == "System.Data.Services.Providers.DataServiceProviderMethods")
return true;

if (methodInfo != null && methodInfo.Name == "CompareString" &&
Copy link
Member

@hazzik hazzik Aug 6, 2017

Choose a reason for hiding this comment

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

This is normally handled by VBStringComparisonExpression. The VB Core/New Roslyn case will be fixed in re-linq 2.2 (it's in alpha-3 now)

Copy link
Contributor Author

@ngbrown ngbrown Aug 7, 2017

Choose a reason for hiding this comment

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

Ok. Is this work-around fine for now, or should NHibernate depend on the alpha 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 think, test with 2.2-alpha3 and wait for the official release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work-around will have to stay till re-linq 2.2.0-alpha-004.

@hazzik hazzik added this to the 5.1 milestone Aug 6, 2017
There's no documented guarantee that IEnlistmentNotification.Prepare will be called from a different thread.
@hazzik hazzik merged commit e68bcb8 into nhibernate:master Aug 14, 2017
@hazzik
Copy link
Member

hazzik commented Aug 14, 2017

Don't want to wait as VS2017.3 is released.

@hazzik hazzik modified the milestones: 5.0, 5.1 Aug 14, 2017
<Import Include="System.Diagnostics" />
<Import Include="System.Linq" />
<Import Include="System.Xml.Linq" />
<Compile Remove="Issues\NH3302\**" /> <!--"Like" not supported in new dotnet compiler, not implemented anyways in .net framework-->
Copy link
Member

@hazzik hazzik Aug 15, 2017

Choose a reason for hiding this comment

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

Just noticed this. '"Like" not supported in new dotnet compiler' is not actually true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I meant that it's not implemented in the full .NET Framework version of NHibernate. At least it's ignored in the tests:

        <TestFixture(), Ignore("Not fixed yet.")> _

I don't actually know if it has been fixed, as I didn't try running it. Either way, the whole file doesn't compile with dotnet build.

Copy link
Member

Choose a reason for hiding this comment

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

It's because Microsoft.VisualBaaic assembly is not referenced

@fredericDelaporte
Copy link
Member

Don't want to wait as VS2017.3 is released.

Anyway it is already released, but not detected as an update from Visual Studio IDE. Launching the Visual Studio Installer detects it and allows installing it (at least from a 17.2).

@ngbrown ngbrown deleted the NH-3990-VB branch September 15, 2017 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants