Add XFile::GetDirectory tests

[DanRStevens]

This should close #215.

Please review or adjust the tests based on expected return values.

Feel free to expand the tests for missing cases.

---

For the test names, I was uncertain if I should put the "GetDirectory" part as part of the first parameter or the second parameter. I would appreciate some feedback on that.

Ex:

TEST(XFileGetDirectory, EmptyPath) {

Versus:

TEST(XFile, GetDirectoryEmptyPath) {

[DanRStevens]

Ahh, and @Brett208, if you can, please add the new test file to the test project. I notice the new tests didn't run on AppVeyor. I added a Windows specific code block, so this should be checked.

[DanRStevens]

Thank you for updating the project file.

Looks like we have some path separator differences on Windows. The test code was written to assume the portable forward slash separator was returned, while the platform specific backslash separator was returned on Windows.

I suppose we need to decide if the returned strings should have platform specific separators, or if they should try to use portable separators as much as possible. The only time you really need platform specific separators is if you are passing the string as part of a command to the shell. In the past, I've generally done such conversions only if and when they are needed, and preferred to use the portable forward slash separator otherwise.

Mind you, if we are basing this code on the C++ Filesystem library, and if that library uses platform specific separators by default, we may want to just go that route.

[Brett208]

Just tested the new update. With this change, the function:

std::vector<std::string> XFile::GetFilesFromDirectory(const std::string& directory)

is no longer returning any files. The passed value for directory causing the problem is / on Windows.

-Brett

[DanRStevens]

Ok, seems we have some cascading changes that will need to be addressed. Probably a good idea to go through other methods anyway, and handle things consistently. I'll take a look into this.

[DanRStevens]

Hmm, seems the CI builds don't include Google Mock. I believe it was integrated into the Google Test repository at some point, though was previously separate. Perhaps the installed versions on the CI machines are from before the merger.

@Brett208, If it's not too much trouble, could you add GMock or a combined GTest and GMock package for Nuget? Nuget GMock packages

I believe the most recent version of Google Test/Google Mock is 1.8.1.

I'll have to look into updating the TravisCI build. Or perhaps just migrate some of that to CircleCI.

[Brett208]

I tried adding the package googlemock.v140.windesktop.static.rt-dyn locally. I had to replace all #include "gmock/xxx.h" with "gtest/xxx.h" and then everything worked. Of course changing the google mock header files isn't really a viable option for our integrated environment.

Frustrating that neither Microsoft or Google is providing an up to date GTEST/MOCK Nuget package.

Will give it another go a little later on.

-Brett

[DanRStevens]

I downloaded a few packages and took a look at them. Seems they are just ZIP files. Microsoft.googletest.v140.windesktop.msvcstl.static.rt-static - (current package) GTest only. Version 1.8.0. googlemock.v140.windesktop.static.rt-dyn.nuspec - GMock only, but in a "gtest" folder. Version 1.7.0. googletestmock.v.140 - Both GTest and GMock in respective "gtest" and "gmock" folders. Version unknown (seems to list a package version of 1.0.1, rather than a Google Test version number). gmock - Both GTest and GMock in respective "gtest" and "gmock" folders. Version 1.7.0.

We could also consider adding GTest as a Git submodule.

[Brett208]

I tried the following package.

<package id="googletestmock.v.141" version="1.0.3" targetFramework="native" />

With it, I have to switch our runtime library to multi-threaded DLL for both our test suite and OP2Utility. I also had to add the following function:

#include <gtest/gtest.h>

int main(int argc, char **argv)
{
    testing::InitGoogleTest(&argc, argv);
    return RUN_ALL_TESTS();
}

Otherwise it worked without having to change any paths.

---

So, we need to find a package that targets the static (non-DLL) runtime library and places gmock.h's code in the folder gmock instead of gtest. It shouldn't matter if the nuget package uses an older toolset unless something happens like in mini-upnp where a breaking change is encountered between the toolsets. I think that is supposed to be rare/edge case.

If that doesn't exist, we could theoretically create our own nuget package. Not sure if we would want to create some sort of group account. It is free and doesn't really seem to require any validation.

If we want to add gtest as a submodule, I believe we will need to create our own build environment for Visual Studio/MSVC using cmake. This could be done in a fork of gtest or maybe right inside of OP2Utility.

To add to the confusion, there is a program called vcpkg that could also manage packets for us. Not sure it is worth switching to. Not sure which is the 'more official' way to do things. I think nuget is more mainstream though? https://github.com/Microsoft/vcpkg

Open to trying either our own nuget package or gtest as a submodule. Not sure we want to pursue vcpkg. If you have a promising nuget package that you want me to try, I'm willing to do so.

-Brett

[Brett208]

I tried installing vcpkg. I read some documentation saying it is automatically installed via appveyor, so should be compatible. Using vcpkg, I installed gtest. It linked fine to OP2Utility, except i got the following error:

Error LNK2001 unresolved external symbol "class testing::internal::Mutex testing::internal::g_linked_ptr_mutex" (?g_linked_ptr_mutex@internal@testing@@3VMutex@12@A) OP2UtilityTest C\OP2Utility\test\XFile.test.obj 1

It seems like this may be a common error: https://github.com/google/googletest/issues/292. Possibly related to static building.

Anyways, if we could fix this issue, we may be able to use vcpkg for our purposes.

-Brett

[DanRStevens]

I'd like to avoid forking GTest. I don't think that is necessary. Packaging could be done as an external concern.

Having an organizational Nuget account might be good.

Any information on the errors when you leave project settings to build a static library? It seems odd the other two projects would need that setting changed to match GTest.

I'm looking into vcpkg. I noticed it can run natively on Linux. It failed to build with Clang, but seems to compile with g++.

[Brett208]

Creating a nuget account for an organization requires an email address that is not used by any of the members of the account. Not sure if we have an email account that works in this capacity. Otherwise seems easy. https://docs.microsoft.com/en-us/nuget/reference/organizations-on-nuget-org

I cloned googletest today. Was able to use Visual Studio with the provided cmake project to compile .lib files for gtest and gmock fairly effortlessly. Unsure if what I compiled is dynamically or statically linked? probably a way to set with cmake. I compiled in x64 release with debug info. Not sure if we would want to compile in x64 or x86 or both?

Not really feeling up to the task of creating a nuget packet from scratch or somehow getting the compiled code to work as a submodule. Maybe this needs to be some sort of paired programming session.

-Brett

[DanRStevens]

Alright.

What if we use one of the Google Test version 1.7.0 Nuget packages that includes GMock? I don't think we need the latest features from Google Test or Google Mock.

For now, I'll see what I can do about the failing TravisCI build. Either fix Travis, or make CircleCI take over for that.

I could also rewrite the new tests to not make use of GMock.

[Brett208]

Quick update. I compiled gmock/gtest on my own using static code generation. I then removed the gtest nuget package. After that, I was able to successfully link to the compiled code and run our test suite. Caveat is I haven't done it on the branch that contains googlemock.

Give me some more time and I'll try to push a release that includes compiled x86, x64 builds in both debug and release with the header files included. This will allow removing the nuget package altogether. If we like it and think it does great things for the world, we could look at processing it into a nuget package.

-Brett

[DanRStevens]

It looks like you've simply added the source code for Google Test into our repo. I would be hesitant to do this. I think a Git submodule might be a bit cleaner. At the very least, that would allow for more automatic updates.

[DanRStevens]

Perhaps we should make Google Test configuration changes on a new branch, and merge it here when done.

[Brett208]

I added the files in the gtest and gmock include folders (headers and pump files) and the compiled .lib/.pdb files. Since the source code in the src folder and project files are not included, you wouldn't be able to compile. This is similar to how freeimage was added. We might be able to pair out some of the header files, it seems like a lot in the include directories.

Not quite sure what a pump file is, but it seems related to templates, possibly a google specific concept though. I left them in since they were in the include directory.

I put the code here to test if gmock was working properly since we don't call it outside this branch. No problem on my end if we want to move it.

-Brett

[Brett208]

Oh, I compiled against the most current copy of GoogleTest. It might have made more sense to use the 1.8.1 release tag to compile against instead of a developmental copy. I'm happy with what was done or can try recompiling on the last actual release at some point.

-Brett

[DanRStevens]

I just did a fresh clone of the repo, and it was kind of slow. The additional .lib and .pdb files have really ballooned the size of the repo. I'm not sure it makes sense to store the .pdb files in the repo here. Storing the .lib I can kind of understand, but I feel like this is a less than ideal solution.

I think I would much prefer platform specific config to download dependencies, rather than platform specific binaries included in the repository.

I'm afraid I haven't got back to the core issue on this one yet.

[Brett208]

I wondering how you would feel about the pdb files. I'm making this up as I go. No issue if we want to delete the pdb files.

We could also probably delete the libs that have _main. I think they are just used if you want to include a main function in your test code but I haven't verified that behavior yet.

I was looking at the Nuget packages more last week. You are right they look pretty simple. We could look at stuffing the lib files in our own Nuget package. No time today but maybe in a day or two.

What happened that broke the Linux builds?

Brett

[DanRStevens]

The _main libs are for when you don't want to provide your own custom main to run the tests. Just use the library provided main. We do actually use those libs.

I'm ok with taking this one slow to get things done right.

The TravisCI builds have basically the same issue as on Windows, they use an old package which doesn't include gmock.

test/XFile.test.cpp:3:10: fatal error: 'gmock/gmock.h' file not found
#include <gmock/gmock.h>
         ^~~~~~~~~~~~~~~
1 error generated.
make: *** [.build/testObj/XFile.test.o] Error 1
The command "make check" exited with 2.

[Brett208]

Ok, I guess I had assumed you had GoogleMock working on the Linux builds before I started messing around.

I made a nuget package today. It seemed well formed. However, when I attempt to upload it to the Nuget site, it finishes uploading and doesn't appear to do anything. No warning message, but doesn't provide feedback either. I tried in 2 different browsers. Max package size is quoted as 250MBs, so we are well below that.

-Brett

[DanRStevens]

Google Mock works on my local machine, and on the Docker image I built for CircleCI. It doesn't work on TravisCI, which uses an old version of Ubuntu, with an out of date system package. I haven't gotten around to updating the TravisCI build.

Curious about the Nuget upload. Is there perhaps a delay between upload and publishing? If they are distributing binaries to be mass installed on machines on the internet, perhaps they run virus scans and stuff first before publishing it.

[Brett208]

@DanRStevens This has been sort of outstanding for quite a while. What do you think about redesigning the tests to just use GoogleTest?

The other option would be I could spend more time learning and testing the nuget package to try and get something published.

It might be better to just get this working to resolve OP2MapImager being buggy. We can force remove my last 3 commits to reduce the repository size. Perhaps in the future VS will better support Google Mock out of the box.

-Brett

[DanRStevens]

This has been outstanding for much too long, and weighing on my mind the whole time.

I've split the changes to add GoogleMock into a new branch, and then removed it from this branch. We can decide to fix up, or scrap that work independently.

Sounds like building and maintaining our own NuGet package for Google Test is a bit of a pain. I'd like to avoid that if possible for now. (Though learning how to publish NuGet packages could be valuable in its own right).

I'm going to experiment a bit with getting GoogleMock to run in the TravisCI environment. I suspect it could be as easy as adding a new apt package. Though if we can't get GoogleMock to run on Windows, this will be a bit of a dead end.

I may need to revisit the test code to see how I might rewrite it to avoid GoogleMock.

[DanRStevens]

Ok, I merged in some changes that add Google Mock on TravisCI. That fixes the TravisCI build failures. Left are the Windows failures due to a missing Google Mock dependency.

Merging master also resolved the merge conflict that resulted from this branch getting stale.

---

Outstanding: Do one of the following:

• Get GoogleMock working on Windows
• Rewrite tests to avoid using GoogleMock
• Split problems tests into another branch, deferring the problem

For the last one option, we'd still need to fix the bug with GetFilesFromDirectory introduced by the first few fixes in this pull request for GetDirectory.

[DanRStevens]

I had a couple of additional thoughts.

---

We never much explored the vcpkg option. I'd like to learn more about what you did with it. Did you have something working using vcpkg? At the time I didn't know enough about it to really comment.

I looked back at some old notes. I had downloaded and compiled vcpkg. My first attempt at compiling produced some compile errors because I had my CXX environment variable set to clang++-6 rather than the usual g++. The vcpkg project was only designed to compile with g++. I'm looking at the bootstrap.sh script now, and it's very clear it's only designed for g++. I have been able to compile it with g++.

If you have further instructions on usage, that would be appreciated.

---

The error reported with GetFilesFromDirectory was never all that clear to me. In particular, where was it called from, and with what argument? I think one of the reasons why I wrote the Google Mock tests, was to figure out what the error actually was. I never saw a clear demonstration of the problem. I suppose we could diagnose and fix the problem without using Google Mock.

[Brett208]

I just found a bug in the ResourceManager which either solved the issue or partially solved it. I'll sit down and look at testing some more when I have more time to see if there is still a problem.

My bust on causing the bug in the first place. Sorry.

-Brett

[DanRStevens]

I was just looking back at the original change for GetDirectory, and the subsequent error report for GetFilesFromDirectory. The path mentioned was /, though which utility this affected was not mentioned.

I had originally assumed the error was caused by cascading changes. It seems no code within OP2Utility calls GetDirectory. In particular, GetFilesFromDirectory does not call GetDirectory. There is no direct dependence, so perhaps there is an indirect dependence within one of the programs that uses the library.

I checked OP2MapImager and OP2Archive. They were both using GetDirectory and then passing the result to initialize ResourceManager. There was no other significant uses of GetDirectory results, only one use in an error message in OP2Archive. The recent bug fix applied to OP2MapImager removed it's only use of GetDirectory. The one remaining use is in OP2Archive, in ConsoleFind.cpp.

From ResourceManager, there are calls to GetFilesFromDirectory. Hence there is an indirect dependence of GetFilesFromDirectory on the result of GetDirectory, but only through the ResourceManager object, and currently only in OP2Archive.

Within ResourceManager there are 4 calls to GetFilesFromDirectory. Two of them are used in the constructor, with the supplied archiveDirectory path, to find the ".vol" and ".clm" files. The other two uses are in GetAllFilenames and GetAllFilenamesOfType to find map and saved game files. The "GetAllFilenames" methods are only used in OP2MapImager, and were used with the same path used to initialize the ResourceManager object. The "GetAllFilenames" methods are not used in OP2Archive.

Given the recent bug fix in OP2MapImager, I suspect the bug report here may have been unrelated to the original change.

---

Given that, perhaps we should should disable the Google Mock dependent tests for GetFilesFromDirectory, which were added after the bug report. If there is no current bug, then this branch could be merged. Enabling or rewriting the Google Mock tests could be deferred until later.

[DanRStevens]

Ok, I'm merging this monster now. We can tackle any other bugs/renames/de-duplication as separate issues.