GEODE-4921: Initial revision of new testing framework. #249
Conversation
jake-at-work
commented
Mar 22, 2018
jake-at-work
commented
- Single process.
- Easy to debug from IDE.
- No environment variables required to run.
- Uses GTest as base framework.
- Single process. - Easy to debug from IDE. - No environment variables required to run. - Uses GTest as base framework.
|
Wow, really nice to see this! Are there any particular areas you're looking for review? I intend to make some time to take a look over the weekend. |
|
Not looking for anything particularly other than if this a good a starting point. Intentionally didn’t try to flush out the whole framework. I expect this will mature and evolve as needed. |
|
Looks like I still have some issues on Solaris and Linux. This should be working great on Windows and MacOS. Will close until I get those resolved. |
There was a problem hiding this comment.
Cool stuff. Just a couple comments to get an idea for where this is headed.
Random (and perhaps unrelated) thought - I see the value of integration tests as sort of a way to test all the pieces working together. But has there been any thought into how to refactor cppcache so that some of existing integration tests can be broken down and written as isolated unit tests?
| #include <iostream> | ||
| #include <random> | ||
| #include <thread> | ||
| #include <future> |
There was a problem hiding this comment.
future. Sounds fancy. And looks like fun. ;)
| target_link_libraries(_apache-geode INTERFACE | ||
| Dbghelp | ||
| ) | ||
| target_compile_definitions(_apache-geode INTERFACE | ||
| # Required for Boost.WinAPI | ||
| _WIN32_WINNT=0x06020000 |
There was a problem hiding this comment.
Some Boost dark magic! I couldn’t find any other good place to add it. It’s required to set the minimum NT version Boost.WinAPI will compile for. In this case NT 6.2.
There was a problem hiding this comment.
It popped up with the Boost 1.66
| .region() | ||
| .withName("region") | ||
| .withType("REPLICATE") | ||
| .execute(); |
There was a problem hiding this comment.
I like the fluent interface. But I wonder how is this going to scale out with different configurations?
After seeing past mistakes in this code base, I worry about different tests having slightly different configurations and seeing copy/paste commits that introduce obfuscated dependencies and bugs in the tests. Any thoughts for how we might be able to design this new test framework to help avoid/deter this?
There was a problem hiding this comment.
Yeah, I see that concern. I was more concerned with avoiding our current framework tat hides away dozens of different configurations in base and helper classes. Here the preference is towards expressiveness. If you notice that command is nearly identical to the actual gfsh command you would type to create a region.
| .withType("REPLICATE") | ||
| .execute(); | ||
|
|
||
| auto task1 = std::async(std::launch::async, [&] { |
There was a problem hiding this comment.
Have you run into any limitations from switching to single process? Do you imagine that all dunit tests can be ported to this new framework?
There was a problem hiding this comment.
No issues other than bugs in the client that it already flushed out. We claim to support multiple caches in the same process so this will make sure that holds true. The old framework had to create separate processes only because the cache was a global variable.
Yes all should be portable. As you can see I ported one and it took just a few minutes.
|
Yes. The goal is to migrate and refactor more code to unit tests. The other goal is to move away from the complicated existing integration framework and embrace a single process model now that we can have multiple caches per process. |
|
I'm curious about spinning up a cluster for each test. When I've done other database type testing, I have usually just made sure to clear the database in a before each or after each to save start up slash shut down costs. Is there any concern here about that? I of course gladly take the win of easier debugging, single process, other things you already mentioned over my little concern. |
|
@moleske I haven’t seen many developers successfully execute integration testing where a shared resource was modified. Too many times I have seen tests written dependent on the previous tests completing successfully. Those bad tests lead to inabilities to parallelize, reorder or run in isolation. I personally prefer to set an example of clean and isolated integration tests. That all said this framework does not enforce any resource management on the tests or test cases. |
|
Great points, there can be bad practices like that. Would love if we can randomize the order of the tests run each time to prevent that. |
|
Because we run them using CTest and in parallel they are effectively randomized but can’t share resources since they are all launched in separate processes. Using the gtest runner they all run sequentially based on their defined order at compile time (no parallelism support). |
- Single process. - Easy to debug from IDE. - No environment variables required to run. - Uses GTest as base framework.
