Add gtest test project

[DanRStevens]

We should add a Google Test test project to this repository. Many of the helper methods could use some unit tests.

---

As this is a Windows only project, we'd need to use Mingw to compile from Linux. This requires a bit of special setup for Google Test. In particular, when building Google Test, we must set the command line option to cmake: -DCMAKE_SYSTEM_NAME="Windows". Additionally there are some pthread issues when compiling gtest with Mingw. These are avoided by running cmake with the command line option -Dgtest_disable_pthreads=ON. These options can be added to the makefile.

There are additional complications when compiling the test code (as opposed to Google Test itself). To reference the Google Test library, we use #include <gtest/gtest.h>. The system installed headers are found in /usr/include/gtest/, hence we need an include directory of -I/usr/include. Unfortunately, this brings in standard Linux system header files, which conflict with the Mingw specific header files for Windows cross compilation. To get around this, we can change the include directory to be more specific with -I/usr/include/gtest/, and then update the source code inclusion to #include <gtest.h>. Unfortunately, Google Test itself includes it's other header files by using gtest/ as part of the path, so those headers won't be found. The solution I found was to copy the /usr/include/gtest/ folder to another location, perhaps .build/include/gtest/, and then add -I.build/include/ as an additional include folder. That way it brings in gtest, but not the rest of the Linux system libraries.

This would be easier if Google Test didn't depend on the parent folder of the gtest/ folder being in the include path.

[Brett208]

I'll take care of adding a VS gtest project and moving the code around.

Understand if you want mingw working for Linux tests and to be more productive. I would say not really required since we only plan to compile with MSVC for this project.

-Brett

[DanRStevens]

Tagging: Windows project structure updates: PR #51 PR #53 Linux makefile updates: PR #48 PR #52 Other: PR #46

---

Ok, in hindsight, I think we followed kind of a bad procedure on this one. It's resulted in a bit of a twisted interdependent mess. We had a large organizational change to add the test project while another branch was open. That creates merge conflicts. I was originally going to ask to merge the other branch first, but figured since the work here was already done, I'd just try and merge it. Additionally, as I wanted to add tests for the other branch, we needed a test project to run them from. Hence the mess of interrelated conflicting updates. As we've run into an issue with the test project setup, now multiple branches are kind of messed up.

I think maybe we need to take a step back and have another go at this.

As testing is already working on Linux, and the other branch has tests added, which were passing on Linux, perhaps we should merge that branch first. We need to back out of the test project merge though, and redo the function move, which I think we want to keep.

From the test update branches, we found having newlines at the ends of certain files and running dos2unix first helped simplify and smooth out the process. That's easy enough to redo and merge in.

There were also some project file cleanup, which I think we want to keep.

I'm a little bit uncertain of what directory structure we want going forward. I think that's the part we need to figure out. It looks like the op2ext source may need to be split into a static library and a dynamic link library that references the static library portion of it. Am I certain of that? No. But that's what seems to make sense at the moment. How it's going to affect the project layout, I'm a little uncertain.

[DanRStevens]

Curious question. When you compile op2ext as a DLL, does it also produce a .lib file? If so, what happens if you add a reference to that .lib file from the test project?

If not, is there a way to get a DLL build to also produce a .lib file?

---

This reference is maybe not great, but might have something useful in it: https://docs.microsoft.com/en-us/visualstudio/test/how-to-write-unit-tests-for-cpp-dlls?view=vs-2017 A number of the methods listed seem to have some big downsides. There is mention of adding a .lib file from the DLL project as a dependency of the test project. Alternatively, you can also add the object files as dependencies. That might be more maintenance than adding a single .lib file though, especially if new source files are added.

[Brett208]

Yes, it produces a small lib file. Attached is the .lib produced from the current master branch in release mode. op2ext.zip

[DanRStevens]

I checked the contained symbols. It doesn't contain much. Just the exported functions.

nm op2ext.lib

op2ext.dll:
01016991 a @comp.id
00000000 i .idata$2
00000000 i .idata$4
00000000 i .idata$5
00000000 i .idata$6
00000000 I __IMPORT_DESCRIPTOR_op2ext
         U __NULL_IMPORT_DESCRIPTOR
         U op2ext_NULL_THUNK_DATA

op2ext.dll:
01016991 a @comp.id
00000000 I __NULL_IMPORT_DESCRIPTOR

op2ext.dll:
01016991 a @comp.id
00000000 I op2ext_NULL_THUNK_DATA

op2ext.dll:
00000000 T _AddVolToList
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__AddVolToList
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T .text

op2ext.dll:
00000000 T _GetConsoleModDir_s
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__GetConsoleModDir_s
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T .text

op2ext.dll:
00000000 T _GetCurrentModDir
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__GetCurrentModDir
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T .text

op2ext.dll:
00000000 T _GetGameDir
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__GetGameDir
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T .text

op2ext.dll:
00000000 T _GetGameDir_s
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__GetGameDir_s
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T .text

op2ext.dll:
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
00000000 I __imp__Log
         U __IMPORT_DESCRIPTOR_op2ext
00000000 T _Log
00000000 T .text

op2ext.dll:
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
         U __IMPORT_DESCRIPTOR_op2ext
00000000 I __imp__SetSerialNumber
00000000 T _SetSerialNumber
00000000 T .text

op2ext.dll:
00000000 I .idata$4
00000000 I .idata$5
00000000 I .idata$6
         U __IMPORT_DESCRIPTOR_op2ext
00000000 I __imp__StubExt

---

I don't see any articles online that discuss producing both a DLL and a full static library along with it. I'm guessing it's not possible.

If we want to support unit testing, we need to do a static library build. To maintain proper testing of what goes into the DLL, we'd need to have a separate DLL project. The DLL project can be a minimal wrapper that just depends on the static library code. Perhaps op2ext.cpp and op2ext.h can go in an op2ext-dll project, while the rest goes into an op2ext-static project. Add to that the test project, and we'd have 3 projects in that solution.

If the DLL project only contains the two source files, do we really want to split off an include folder?

---

Perhaps I'm being overly cautious about the project re-organization. It might be some of those changes were still moving in the right direction, even if things didn't quite work yet.

[Brett208]

Based on the article https://docs.microsoft.com/en-us/visualstudio/test/how-to-write-unit-tests-for-cpp-dlls?view=vs-2017, I think creating a static configuration on top of the DLL configuration as we have done is acceptable. (However, the article does read vaguely on this point). At the same time, I am not staunchly opposed to solving this by creating an additional library, besides the extra work that I am not planning on putting into the project right now.

The unit tests call non-member functions which are not exported from the DLL, and the DLL can be built as a static library: Change the DLL project so that it is compiled to a .lib file. Add a separate test project that references the project under test.

This approach has the benefit of allowing your tests to use non-exported members, but still keep the tests in a separate project.

Go to the procedure To change the DLL to a static library.

That being said, if we want to improve unit tests on op2ext, I think it is more important to redesign large chunks of the code base to be testable without Outpost2.exe being present than creating a library project. As right now even if we created a library project, I don't think the console mod loader, ini mod loader, ip dropdown, or vol list could be instantiated and tested. Issue #19 could assist in running the code without an actual copy of Outpost2 available.

However, I am fine focusing on NetFix right now and just adding unit tests to op2ext as we go where it makes sense.

In that vein, I would be happy to create a ReleaseStatic configuration and add it to AppVeyor. Someone would still need to solve the IF statement issue in the appveyor yml file, which I have not cracked yet. We could then move on for now with limited victory.

If you wish to rewrite op2ext to include a library project, I will support by configuring the MSVC project file and hooking it properly into the solution and testing on Windows. That effort shouldn't be difficult. But, I am not planning on putting the effort into completing the actual separation of the code into 2 projects (and I'm not sure that I am well equipped with my current toolkit to accomplish anyways).

[DanRStevens]

I don't think I've explained my idea very well. It sounds like I've given you the impression it would be a big abstract change. I suppose I was thinking that way myself originally. In practice, it should be just a small re-organization, involving about 2 files, and 2 project settings.

At this point, we probably already have enough sample project files to work from. Perhaps I can patch something together with just a text editor.

---

I came across that Microsoft article too. They have some interesting strategies, but I don't think any of their cases suit us very well. They are largely hacks to work around a problem without actually directly attacking or solving the real problem. I'm unconvinced that's the best that can be done. It seems they've imposed an artificial restriction of a single project file. We are not so constrained.

---

I don't think adding a static configurations to the DLL project is the way to go. Here's why.

The op2ext project must necessarily be built as a DLL. That's the only way to plug it in to Outpost 2. We are not able to re-link Outpost 2 with a new updated static library. We would need the source code to Outpost 2 to do that.

MSVC is not able to produce both a DLL library and a static library (with the full project contents) in a single configuration. It's one or the other for any given configuration.

Testing depends on both the source code and the project configuration. If we change the configuration, we effectively invalidate testing. That's not to say there won't be strong correlation between two different configurations, or that there is no value in such testing, but rather, such testing falls short of a guarantee. If we're not testing on the same configuration as we are releasing, then we are effectively releasing untested code.

---

Full unit testing can only be done for a static library project. A DLL project necessarily limits testing to the exported DLL interface. The problem then, is how do we fully unit test the library, test with the same configuration as we are releasing, and release a DLL.

The key is that if a DLL project depends on a static library project, the static library contents will be linked into the DLL, as if the static library's source code had been part of the DLL project. Effectively, a new "DLL + Static" project pair will be equivalent to the old full DLL project. The test project is then free to depend on the internal Static library too, exposing the internals for unit testing.

Project dependencies: op2ext-dll -> op2ext-static unitTests -> op2ext-static

Possible directory layout:

op2ext/
  op2ext.sln
  op2ext-dll/
    op2ext-dll.vcxproj
    op2ext.cpp
    op2ext.h
  op2ext-static
    op2ext-static.vcxproj
    // All the rest of the source code
  test/
    test.vcxproj
    // Test code

---

Not only does the above setup allow us to test the internals of the code that goes into Release, but it's also faster too. Building dependent projects is additive to runtime. Building new configurations is multiplicative.

Splitting the project means we still only have Release and Debug configurations to build. Additionally, the static library component is only built once and then re-used for both the DLL and the unit test projects. (They are being done in the same configuration). If instead we double up the configurations, we then double the CI load, as we would then need to build: Release, Debug, StaticRelease, StaticDebug. We'd also have to hope the static configurations stay in sync with the other two corresponding main configurations.

[Brett208]

I have always thought of libraries as a collection of reusable, more generic code. In this case the library project contains large portions of code that will only be usable by op2ext. Compiling it statically is just easing access at an intermediate build step. Even though much of that access isn't even usable by another project due to requiring the presence of Outpost2.exe (including the unit tests).

I'm curious if this is considered an accepted development strategy.

-Brett

[DanRStevens]

Ok, I can see that concern. It's built as a library, but it's so specific and special cased that it's unlikely to be reused elsewhere. I suppose in a way that was why I converted it to a static library, so the code can be re-used by both the DLL and the test project, though that argument feels a bit like a cheat. The code doesn't have much reusability beyond that. I suppose you could use components of it elsewhere, and it wouldn't be wrong to do, though that's very unlikely to happen. Parts of it do have rather specific dependencies on Outpost2.exe. Not all of it though, and I find it hard to say how much can be tested without Outpost2.exe right now. I suppose we'll find out shortly and can decide in hindsight how well this idea works.

[Brett208]

This is more me just musing on my own prejudices for library projects. I think the current path will be successful for allowing unit tests of portions of op2ext. Maybe another bridge to cross someday in the future to test larger swaths.