Fix tests for platforms other than Windows #42
Conversation
|
Yay! I 👍 will look at this soon. Did you tested it on windows, too? |
|
@JohannesHoppe will test on Windows as well and get back to you! |
| @@ -31,7 +32,13 @@ public void DeleteFile(string fullFileName) | |||
| public void MakeFileExecutable (string path) | |||
| { | |||
| //when on linux or osx we must set the executeble flag on mongo binarys | |||
| File.SetAttributes (path, (FileAttributes)((int)File.GetAttributes (path) | 0x80000000)); | |||
| var p = Process.Start("chmod", $"+x {path}"); | |||
There was a problem hiding this comment.
I did run them on Linux, but I confess I fiddled a bit with the project settings to build it for .NET Core 2.0 instead - since that change did a lot of stuff I wasn't sure you actually liked, I kept it out of this PR.
I'm of course happy to submit a separate PR with these changes as well, it might be that some of them are applicable for you.
Re: MakeFileExecutable, I think that code is only executed for Linux as it is (https://github.com/Mongo2Go/Mongo2Go/blob/master/src/Mongo2Go/MongoDbRunner.cs#L135), but to be sure we could have a test and run separate code paths for different OS
There was a problem hiding this comment.
You are absolutely right. I did a quick lookup and haven't seen RuntimeInformation.IsOSPlatform(OSPlatform.Linux). We might want to rename the method from MakeFileExecutable to MakeFileExecutableForLinux! 😄
There was a problem hiding this comment.
to build it for .NET Core 2.0 instead
So you wanted to go from <TargetFramework>netstandard1.6</TargetFramework> to <TargetFramework>netcoreapp2.0</TargetFramework>. Are there any benefits for you to do that?!
There was a problem hiding this comment.
Not entirely sure what I'm doing, I find the combination of versions in the new MS world very confusing! 😄
As you can see in #44, I updated the test project from netcoreapp1.0 to netcoreapp2.0, but it might be that some version in between works as well...
There was a problem hiding this comment.
Yeah. Versions. You will get used to that. It makes sense in the end... 😕 😄
Regarding #44:
Having the Tests in netcoreapp2.0 is no breaking change at all. If they run again, that will be a big benefit! Great! :-)
The Library "Mongo2Go" is still on netstandard1.6. That's great. Increasing the version number here would be a breaking change for people that are still on netcoreapp1.0.
So I was concerned that you wanted to ugprade Mongo2Go.
There was a problem hiding this comment.
I have made the chmod change locally (I have not got round to creating a PR) and it is working fine on Linux. I also renamed the method to MakeFileExecutableForLinux!
There was a problem hiding this comment.
I also added the chmod for macOS. Does anybody here has a Mac!?
|
Man this took more time than expected. New nuget packages might come tomorrow. I need to work! ;-) |
|
@JohannesHoppe I know the feeling! 😄 Thanks for quick review and merge - I know from personal experience that reviewing PRs can take a lot more time than contributors might think. Also thanks for all the other work with Mongo2Go, it has been a really quick way for us to get integration tests going in a big project we're working on. |
|
Thx! Glad to hear that! 😄 |
|
I pushed |
|
@JohannesHoppe Works flawlessly 👍 Thanks again! |
This fixes #37 (1515dda), but also makes the tests run on Linux (and hopefully also OS X).
A lot of paths were hardcoded to use backslash as path separator, which doesn't work on Linux, and also the data path defaulted to use
C:\data\, which obviously doesn't work on Linux.This PR make the somewhat intrusive change of removing the
MongoDbDefaults.DataDirectoryconstant, and will instead by default create the database in the user's temporary directory, which I think is a better default, although I guess it could be debated.