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

Fix PlatformTests #13517

Merged
merged 2 commits into from Jul 6, 2017

Conversation

Projects
None yet
5 participants
@hartmark
Contributor

hartmark commented Jun 16, 2017

Fixed the failing tests in PlatformTest.

The AppDomain.CurrentDomain.BaseDirectory property was a bit of a gotcha because I got a trailing backslash from it when debugging the game, but not in the test.

Perhaps the BUG-comment is redundant, if it is missing the separator we just add it in the property.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 17, 2017

Member

Can you provide logs / explain how the tests are failing? They work fine for me (mono 5.0.1.1 on macOS 10.12), and are also fine on travis and appveyor.

Member

pchote commented Jun 17, 2017

Can you provide logs / explain how the tests are failing? They work fine for me (mono 5.0.1.1 on macOS 10.12), and are also fine on travis and appveyor.

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 18, 2017

Contributor

Two tests fails:

Expected string length 43 but was 42. Strings differ at index 34.
Expected: "C:\Users\musku\Source\Repos\OpenRA\testpath"
But was: "C:\Users\musku\Source\Repos\OpenRAtestpath"
--------------------------------------------------^

at OpenRA.Test.PlatformTest.ResolvePath() in C:\Users\musku\Source\Repos\OpenRA\OpenRA.Test\OpenRA.Game\PlatformTest.cs:line 36

Expected string length 10 but was 11. Strings differ at index 2.
Expected: "./testpath"
But was: "./\testpath"
-------------^

at OpenRA.Test.PlatformTest.UnresolvePath() in C:\Users\musku\Source\Repos\OpenRA\OpenRA.Test\OpenRA.Game\PlatformTest.cs:line 52

Contributor

hartmark commented Jun 18, 2017

Two tests fails:

Expected string length 43 but was 42. Strings differ at index 34.
Expected: "C:\Users\musku\Source\Repos\OpenRA\testpath"
But was: "C:\Users\musku\Source\Repos\OpenRAtestpath"
--------------------------------------------------^

at OpenRA.Test.PlatformTest.ResolvePath() in C:\Users\musku\Source\Repos\OpenRA\OpenRA.Test\OpenRA.Game\PlatformTest.cs:line 36

Expected string length 10 but was 11. Strings differ at index 2.
Expected: "./testpath"
But was: "./\testpath"
-------------^

at OpenRA.Test.PlatformTest.UnresolvePath() in C:\Users\musku\Source\Repos\OpenRA\OpenRA.Test\OpenRA.Game\PlatformTest.cs:line 52

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 18, 2017

Contributor

I'm running Windows 10, Visual Studio 2017

Contributor

hartmark commented Jun 18, 2017

I'm running Windows 10, Visual Studio 2017

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 21, 2017

Contributor

@pchote any more details I need to add?

Contributor

hartmark commented Jun 21, 2017

@pchote any more details I need to add?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 22, 2017

Contributor

@hartmark It seems your changes are not bad, but also not necessary. The tests pass successfully on my Windows 10+VS 2017.

Contributor

rob-v commented Jun 22, 2017

@hartmark It seems your changes are not bad, but also not necessary. The tests pass successfully on my Windows 10+VS 2017.

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 22, 2017

Contributor

Alright, I wonder why we get different outcome on Win10.

I found another one having same problem
https://stackoverflow.com/questions/11720587/why-does-system-appdomain-currentdomain-basedirectory-return-different-results

One guy answered that it has something to do with the appdomains

After digging some more it seems that it has been a source to problems that the apppool is initialized differently.

Here's a bug for corecrl where they say it's a error in the host.
dotnet/coreclr#10920
And that leads me to diagnose how I run the unittests.

It seems to run properly if I run it using visual studio's mstest-tool (had to add NUnit3TestAdapter nuget reference, are we not using Nuget-packages btw?)

And if I run with resharper's testrunner it fails with the error I have written above.

How are you guys running the unit-tests?
@pchote @rob-v @reaperrr

Contributor

hartmark commented Jun 22, 2017

Alright, I wonder why we get different outcome on Win10.

I found another one having same problem
https://stackoverflow.com/questions/11720587/why-does-system-appdomain-currentdomain-basedirectory-return-different-results

One guy answered that it has something to do with the appdomains

After digging some more it seems that it has been a source to problems that the apppool is initialized differently.

Here's a bug for corecrl where they say it's a error in the host.
dotnet/coreclr#10920
And that leads me to diagnose how I run the unittests.

It seems to run properly if I run it using visual studio's mstest-tool (had to add NUnit3TestAdapter nuget reference, are we not using Nuget-packages btw?)

And if I run with resharper's testrunner it fails with the error I have written above.

How are you guys running the unit-tests?
@pchote @rob-v @reaperrr

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 22, 2017

Member

make nunit, which runs nunit3-console.

Member

pchote commented Jun 22, 2017

make nunit, which runs nunit3-console.

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 22, 2017

Contributor

Alright, seems the make.cmd and make.ps1 both lack the nunit command.

Contributor

hartmark commented Jun 22, 2017

Alright, seems the make.cmd and make.ps1 both lack the nunit command.

@aydjay

This comment has been minimized.

Show comment
Hide comment
@aydjay

aydjay Jun 22, 2017

I have also experienced this test failure when running the test using the resharper test runner. I couldn't figure our why the results were differing however.

aydjay commented Jun 22, 2017

I have also experienced this test failure when running the test using the resharper test runner. I couldn't figure our why the results were differing however.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 22, 2017

Member

I think it is reasonable to force a trailing path separator on GameDir. We recently did this for SupportDir in #13525. The changes to UnresolvePath should not be needed though, since you are guaranteed to have the separator removed by the substring.

Member

pchote commented Jun 22, 2017

I think it is reasonable to force a trailing path separator on GameDir. We recently did this for SupportDir in #13525. The changes to UnresolvePath should not be needed though, since you are guaranteed to have the separator removed by the substring.

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 22, 2017

Contributor

I have updated the code per your feedback
@pchote

I have also added Lazy initialization same as SupportDir

Contributor

hartmark commented Jun 22, 2017

I have updated the code per your feedback
@pchote

I have also added Lazy initialization same as SupportDir

@pchote

pchote approved these changes Jun 27, 2017

LGTM now 👍

Just one minor nit:

@@ -11,6 +11,7 @@ obj
*.CodeAnalysisLog.xml
*.lastcodeanalysissucceeded
_ReSharper.*/
/.vs

This comment has been minimized.

@pchote

pchote Jun 27, 2017

Member

Can you please drop or split this to its own commit?

@pchote

pchote Jun 27, 2017

Member

Can you please drop or split this to its own commit?

@pchote pchote added the PR: Needs +2 label Jun 27, 2017

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 27, 2017

Contributor

alright, I'll put that in a separate PR

Contributor

hartmark commented Jun 27, 2017

alright, I'll put that in a separate PR

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 27, 2017

Member

Same PR is fine, just as a separate commit here. Our policy is that a single commit shouldn't do more than one logical change.

Member

pchote commented Jun 27, 2017

Same PR is fine, just as a separate commit here. Our policy is that a single commit shouldn't do more than one logical change.

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jun 27, 2017

Contributor

Aha, alright

Contributor

hartmark commented Jun 27, 2017

Aha, alright

@hartmark

This comment has been minimized.

Show comment
Hide comment
@hartmark

hartmark Jul 2, 2017

Contributor

Updated after @rob-v code comment

Contributor

hartmark commented Jul 2, 2017

Updated after @rob-v code comment

@rob-v

rob-v approved these changes Jul 2, 2017

I apologize @hartmark, I overlooked it, your change with Lazy was fine in fact. With or without it, 👍 .

@pchote pchote merged commit 3a6796a into OpenRA:bleed Jul 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hartmark hartmark deleted the hartmark:fix-unittests branch Jul 6, 2017

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