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

Miscellaneous fixes #695

Merged
merged 7 commits into from Sep 20, 2017
Merged

Miscellaneous fixes #695

merged 7 commits into from Sep 20, 2017

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Sep 20, 2017

Each of these are tweaks that are so small, I don't think they warrant a Jira number for each issue. They are mostly from my NH-3807 and NH-4008 branches, but not necessarily specific to them.

  • Add NHibernate.TestDatabaseSetup to Everything solution.
  • Normalize spaces and tabs in build files.
  • Future proof tests related to WeakReference and upcoming .NET Core changes.
  • Fix test to use entire log instead of splitting on Environment.NewLine
  • Use Path.Combine better for "Tools/PEVerify"
  • Create SqlServer Compact db respecting path in connection string.
  • Update wording in ShowBuildMenu and allow NUnit tests to be ran with x64 mode

@hazzik hazzik merged commit 35bb01c into nhibernate:master Sep 20, 2017
@fredericDelaporte
Copy link
Member

fredericDelaporte commented Sep 20, 2017

Future proof tests related to WeakReference and upcoming .NET Core changes.

There are more cases that are not fixed the same way (no inlining prevention attribute): 1 (and methods at end of file), 2 (not run and not sure it works as expected), 3 (run but maybe not testing anything since what should maybe not be collected is locally created, but not sure, I have not checked thoroughly the tested class and what could be collected with it).

The first may cause issues too I think (and have async counterpart to regen if changed).

@ngbrown
Copy link
Contributor Author

ngbrown commented Sep 20, 2017

@fredericDelaporte WeakHashtableFixture.cs was the only area I ended up having problems (see https://github.com/dotnet/coreclr/issues/12847 for background). I could see 1 potentially having an issue but maybe the source function is big enough it's not getting inlined. The others aren't generating the WeakReference, so I assume the source is across assemblies, which wouldn't get inlined.

start "nunit3-console" cmd /K %NUNIT% --x86 --agents=1 --process=separate NHibernate.nunit
SET NUNITPLATFORM=
IF /I "%PLATFORM%" NEQ "x64" set NUNITPLATFORM=--x86
start "nunit3-console" cmd /K %NUNIT% %NUNITPLATFORM% --agents=1 --process=separate NHibernate.nunit
Copy link
Member

Choose a reason for hiding this comment

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

PLATFORM environment variable looks harmful, it causes build failures due to MsBuild using it if defined, as in here.

It seems anyway that its default is to be not defined, it is just unfortunately set by this ShowBuildMenu when setting up test configuration, causing then build failures (especially on async file generation).

So I wonder if this change works as expected. I think in most cases we are still running nunit as a 32 bits process, even on x64 boxes. Maybe should be changed to use a logic like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NUnit does run in x64 with this change...

The PLATFORM environment variable may be interacting with the async generation elsewhere, but it's not 'harmful' on this line in the ShowBuildMenu.bat. This pull request didn't set the PLATFORM, just tested its value.

Copy link
Member

Choose a reason for hiding this comment

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

NUnit does run in x64 with this change...

Maybe on your machine, not on mine. This PLATFORM environment variable is not a standard thing. Most boxes do not have it. So your change may mostly works on your machine, not elsewhere. That is why I have linked an alternate way using environment variables documented for this usage. (No official doc, but at least many references, like here and other places.)

Copy link
Contributor Author

@ngbrown ngbrown Oct 24, 2017

Choose a reason for hiding this comment

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

The point of this wasn't for general use x64 unit testing, it was only for enabling the testing of the x64 configurations in the build menu (of which there are ones for Firebird, SQLite, and SQL Server Compact). I agree that even at this task it's imperfect as the configurations needs to be created and then the tests ran in the same session of ShowBuildMenu.bat.

It's not about my machine or your machine, as none of my machines ever have PLATFORM set at a machine or user profile level. It was intended to be entirely within the ShowBuildMenu.bat. PLATFORM has been set in the ShowBuildMenu.bat since 2011:

set PLATFORM=x64

Maybe a better solution would have two separate Run tests using active configuration one for each of x64 and x86.

An even better solution would use NuGet packages of the ADO.NET drivers where both sets of native binaries are included in the build process and don't have to be selected at the configuration level, but based on the word size of the currently running process.

Copy link
Member

Choose a reason for hiding this comment

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

Well it is worst than imperfect, since it is a "work only once" solution. Usually we do not re-create configurations for each test run, we just activate the one we want. And activation is not setting this variable.
By the way it cannot work when creating a SQL Server Compact test configuration either: this one put AnyCpu in this variable whatever its bitness.

So yes I agree that two options is the way to go. But that will require a letter shift, like moving B&C to A&B.

About NuGet, see #1401.

Firebird and SQLite data providers are CPU agnostic nowadays anyway. SQL Server CE is still an issue and this one will likely be solved only by dropping it. Oracle stays a special case.

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 was reacting to the comment that this pull request was somehow 'harmful', not that it only barely helps/works.

I think that adding to your #1401 pull request separate menu options to run tests in both x86 and x64 is a good idea.

@fredericDelaporte
Copy link
Member

I was not targeting the PR as harmful, but only that variable, meaning that something else has to be used for handling the feature, because this variable is not suitable. It was not meant as pinpointing any responsibility, as this PR was not introducing it, but just using it.
I was already intending to remove what was defining that variable in this bat, but I was not understanding why the "run test in 64 bit" was relying on this variable. So I have raised the point here.

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.

None yet

3 participants