Add new test application for import backend

[gruberchr]

This new test is motivated by PR #641. Two of the bugs fixed in this PR had no test application before.

[gruberchr]

This is a first draft of the new test application and is work in progress. Feel free to comment on it.

I'm using GMock here the first time for any GnuCash test as I can see. It was quite useful to simplify the test setup.

I have no idea, if the CMakeLists.txt is correct. I was wondering, why this built successfully, since I did not add any further libraries to the test target except gtest and gmock. Probably target gncmod-generic-import already contains all dependencies.

Additionally It wasn't clear to me, what headers to include into gtest-import-backend.cpp.

[jralls]

Test failure is due to emitting # qof.engine-INFO: [qof_event_generate_internal] id=1 hi=0x5604ee86d4e0 han=0x7f869201aaf8 data=(nil) so many times that the test log exceeds the maximum length on Travis CI.

[jralls]

It's not clear to me that this does anything beyond exercising your mocks. Perhaps that's the intent at this stage?

[jralls]

Use angle-brackets to include filepaths not relative to the current directory. It is an incredibly common misconception that angle brackets are for system includes and quotes for project includes: A cursory examination of the compiler manual shows that quotes mean to search the current directory before the include paths while angle brackets go straight to the include paths.

[gruberchr]

Ok, done

[jralls]

Although it's common to use this idiom in GnuCash tests I think it better to put the module-under-test on the include path and use angle-brackets as a relative path buried in an include statement might turn out to be brittle in the face of a code reorganization.

[gruberchr]

done

[jralls]

Say in the comment that it's a GMock MATCHER so that future us will know where the macro comes from.
This won't compile on all supported compilers (it really shouldn't at all), const char* has no operator+(). You want std::string("has ") + (negation ? " no " : "") + "duplicated elements".

[gruberchr]

Changed comment.
Isn't a double quoted string assumed to be std::string in a *.cpp file? I copied from the example in GoogleMock CookBook.

[jralls]

No, absolutely not. A double-quoted string literal is of type const char* and it's instantiated in the code segment. std::string objects must be declared as such or come from a function returning it.

[jralls]

EXPECT_CALL() needs to go in each test, not in the fixture, so that it's in the right stack frame. Perhaps you want ON_CALL() to set up the mock's behavior?

I do wonder if .Times(AnyNumber()) here is what's leading to the infinite loop that causes the test to fail.

[gruberchr]

This is discussed in GoogleMock CookBook. One can also use ON_CALL(), but in this a "Uninteresting mock function call" message is printed (see last paragraph). For me both opportunities are ok.

[jralls]

That doesn't say to put EXPECT_CALL in a fixture. In fact is says the opposite: A good design will use ON_CALL in the fixture to define the mock's behavior and EXPECT_CALL in the TEST_F to test that the code under test does in fact exercise that behavior.

[jralls]

Creating the accounts and the transaction is supposed to go in the fixture's SetUp function, and deleting them in the TearDown function.

[gruberchr]

Is this a requirement in GoogleTest or a convention? I'm still learning to write tests with GoogleTest. IIUC you only add things to the fixture, if you want to use them in more than one test. At the moment the two accounts and the transaction are intended for this one test only.

But of course they have to be deleted, whether here or in the fixture's TearDown() function. I still have to do this.

[jralls]

It's a requirement of basic C++. Any object that allocates should be created in the fixture's setup and freed in the fixture's teardown because those functions are guaranteed to run even if the test throws an exception, whereas deallocation in the test function might not run if the function exits for some reason before the resources are freed. That's different from C test frameworks like GLib's because only a crash will prevent the function from going all the way through.

[gruberchr]

It's not clear to me that this does anything beyond exercising your mocks. Perhaps that's the intent at this stage?

No, that's not correct. Have a look at the test summary, test 88/133 test-import-backend fails. And that's expected, since among other things this test checks for bugs, which should be fixed by PR #641. That means, after merging PR #641 test-import-backend should pass as well.

The problem is, that due to this massive log output the details of test-import-backend are not visible. The log stops at test 23/133 test-backend-dbi. Do you have any idea, what's wrong? New test-import-backend actually shouldn't influence any of the other tests.

[jralls]

My comments were based on code review, not on trying to run the tests.
It turns out that the spew isn't the cause of the failure as you note, but trying to dump the test output is because of the tens of thousands of lines like

# qof.engine-INFO: [qof_event_generate_internal] id=1 hi=0x56356864e7f0 han=0x7f79a7d2624a data=(nil)

from test-app-utils and test-backend-dbi. The reason it's blowing up Travis on Arch is because the tests are getting run there with logging turned way up and when the tests fail the testlog is printed to stdout so that we can have a chance at understanding why. It's not obvious to me where that setting is, perhaps @gjanssens knows, but while it's probably worth it to get detailed info for troubleshooting failures we need to stifle some messages and that's the #1 candidate.

The actual failure on Ubuntu is

Google Mock tried the following 1 expectation, but it didn't match:
/gnucash/gnucash/import-export/test/gtest-import-backend.cpp:239: EXPECT_CALL(imap, findAccountBayes(AllOf( Each(Not(IsEmpty())), Each(Not(HasSubstr(" "))), Not(HasDuplicates()), Contains(StrEq("description")), Contains(StrEq("memo")) )))...
  Expected arg #0: ((only contains elements that isn't empty) and (only contains elements that has no substring " ")) and ((has no duplicated elements) and ((contains at least one element that is equal to "description") and (contains at least one element that is equal to "memo")))
           Actual: { "", "test", "memo", "the", "", "", "test", "", "", "Sunday", "", "", "tokens", "", "description", "", "", "within", "tokens", "", "test", "" }, whose element #0 doesn't match
         Expected: to be called once
           Actual: never called - unsatisfied and active
/gnucash/gnucash/import-export/test/gtest-import-backend.cpp:243: Failure
      Expected: gnc_import_TransInfo_get_destacc(trans_info)
      Which is: NULL
To be equal to: dest_acc
      Which is: 0x83ecc0
/gnucash/gnucash/import-export/test/gtest-import-backend.cpp:239: Failure
Actual function call count doesn't match EXPECT_CALL(imap, findAccountBayes(AllOf( Each(Not(IsEmpty())), Each(Not(HasSubstr(" "))), Not(HasDuplicates()), Contains(StrEq("description")), Contains(StrEq("memo")) )))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] ImportBackendBayesTest.CreateTransInfo (2 ms)

However, when I tried to run it on MacOS using clang I get a segfault:

[==========] Running 2 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from ImportBackendTest
[ RUN      ] ImportBackendTest.CreateTransInfo
* 19:36:51  WARN <GLib-GObject> invalid uninstantiatable type '(null)' in cast to 'QofInstance'
Segmentation fault: 11

I debugged it and found that the underlying problem is

(MockGncImportMatchMap) $15 = {
  GncImportMatchMap = {
    acc = 0x0000000104615ae8
    book = 0x00007ffeefbfd2f0
  }
...

and

(Account) $12 = {
  inst = {
    object = {
      g_type_instance = {
        g_class = 0x0000000104615aa0
      }
      ref_count = 646032
      qdata = 0x00000001059109d0
    }
    e_type = 0x0000000105910c48 ""
    kvp_data = 0x000000010009db90
  }
}

That kvp_data looks like

(KvpFrame) $7 = {
  m_valuemap = size=4294967295 {
    [0] = {
      first = 0x10ec8348e5894855 <no value available>
      second = 0xf87d8b48f87d8948
    }
    ...
  }
}

Those addresses aren't in the legal address space so when first gets dereferenced in qof_instance_get_path_kvp it quite understandably segfaults.

I suspect that it doesn't segfault with gcc it's getting initialized to all 0s and not getting initialized on clang (which would be a bug because according to the standard everything except POD should be initialized to 0 unless there's a constructor to do something different). Even if it is a clang bug I don't think that googlemocking a C struct is going to work: I think googlemock is intended to mock C classes and in particular instance member functions, so that failure in Ubuntu above is because there is no code anywhere that calls GncInstanceMap::findAccountBayes, nor can there be.

I'm travelling this week with no access to my Linux VMs so I can't test that hypothesis any further until I return Sunday and I can't promise to have time to pursue it before the 3.9 release at the end of March.

[gjanssens]

My comments were based on code review, not on trying to run the tests.
It turns out that the spew isn't the cause of the failure as you note, but trying to dump the test output is because of the tens of thousands of lines like

# qof.engine-INFO: [qof_event_generate_internal] id=1 hi=0x56356864e7f0 han=0x7f79a7d2624a data=(nil)

from test-app-utils and test-backend-dbi. The reason it's blowing up Travis on Arch is because the tests are getting run there with logging turned way up and when the tests fail the testlog is printed to stdout so that we can have a chance at understanding why. It's not obvious to me where that setting is, perhaps @gjanssens knows, but while it's probably worth it to get detailed info for troubleshooting failures we need to stifle some messages and that's the #1 candidate.

Unfortunately I don't know how to suppress those messages. I have spent a few hours this morning figuring out how to do this but didn't get anywhere.

What I did manage was to simplify the error output by changing our travis test script: the script will now only output the logs from failed tests, not all tests. That should already help in many cases (like yours).

If you rebase your branch on current maint and then force push it to your github repo, the new travis build log should be much reduced.

[jralls]

@gjanssens that's a nice improvement. Thanks.

[gruberchr]

I rebased my branch on current maint. But since PR #641 has been merged now, new test-import-backend shouldn't fail anymore. And since PR #641 has been merged just before changes from @gjanssens, there is no commit, which could be used to check current travis behaviour in case of a failed test.

[jralls]

Wrong, the rebase was sufficient to trigger CI and it now passes. That doesn't affect any of my comments and until you address those, most especially the incorrect use of googlemock with GncImportMatchMap this isn't going anywhere.

[gruberchr]

Regarding MockGncImportMatchMap and Clang:
It's correct, that the members acc and book of MockGncImportMatchMap aren't initialized, that should be changed. But I don't understand, in which code line these members are accessed, so that QofInstance() is invoked. Actually the map should only be passed as is to the mock function gnc_account_imap_find_account_bayes() in gtest-import-backend.cpp. Or is this maybe a linker problem, that not the mock function is linked, but the function from libengine?

I'm aware that mocking a C struct as a C++ class to work with GoogleMock is not the standard case. But I guess that all new tests should be written with GoogleTest/GoogleMock anyway. I tried a lot to get this test run and hope it wasn't for nothing. Mocking PrefsBackend wasn't straightforward as well.

[jralls]

You're passing a ptr to your mock to gnc_import_TransInfo_new(), where it gets handed around and its guts dereferenced by the various C functions that gnc_import_TransInfo_new() calls. If the ptrs in the mocked struct are nullptr then the nullptr guards in those functions just return without doing anything besides logging an error. If they're not initialized, i.e. they're set to whatever bits were left in that memory when it was last used, a crash on dereference is inevitable.

GMock is clearly not intended to mock C structs or GObject objects (especially not GObject objects!) and I don't think that trying to use if for that is a good design.

[jralls]

Actually the map should only be passed as is to the mock function gnc_account_imap_find_account_bayes() in gtest-import-backend.cpp. Or is this maybe a linker problem, that not the mock function is linked, but the function from libengine?

And how is that supposed to happen? You don't do so explicitly and providing a new function definition in your test program isn't going to magically re-link libgncmod-import-generic.so so that gnc_import_TransMap_new will call it.

[gruberchr]

And how is that supposed to happen? You don't do so explicitly and providing a new function definition in your test program isn't going to magically re-link libgncmod-import-generic.so so that gnc_import_TransMap_new will call it.

I don't think that this is magic. Original function gnc_account_imap_find_account_bayes() is contained in libgncmod-engine.so and is called from function matchmap_find_destination(), which is contained in libgncmod-generic-import.so. That means linking both functions together is not done, when building libgncmod-generic-import.so but when building application test-import-backend. That means nothing should be re-linked. And this means, that linking the mock function gnc_account_imap_find_account_bayes() from gtest-import-backend.o instead of the one from libgncmod-engine.so is a question of the lookup order during the linking process. And IIUC the linker should lookup in gtest-import-backend.cpp.o at first, because this object file is placed before libgncmod-engine.so on the command line. But to be honest, I'm not an expert on questions related to the linking process, especially not on dynamic linking.

At least this works for me and even on Travis CI. Otherwise the test would fail, since it expects the mock functions MockGncImportMatchMap::findAccount() and MockGncImportMatchMap::findAccountBayes() to be called once respectively.

Finally if linking the mock functions instead of the original functions can not be safely controlled, it would be quite hard to mock any C function regardless of the used framework.

[jralls]

Sorry, my comment about magic was a bit harsh. You're probably familiar only with the gcc linker.

Original function gnc_account_imap_find_account_bayes() is contained in libgncmod-engine.so and is called from function matchmap_find_destination(), which is contained in libgncmod-generic-import.so. That means linking both functions together is not done, when building libgncmod-generic-import.so

That's how it works on Linux. It's not how it works on MacOS, which generates two-level symbols that contain both the function name and the shared library that it was found in at link time. Here's the relevant frames from the debug trace:

    frame #4: 0x000000010044a10c libgncmod-engine.dylib`gnc_account_imap_find_account + 636
    frame #5: 0x00000001000aa52b libgncmod-generic-import.dylib`matchmap_find_destination + 123
    frame #6: 0x00000001000aa418 libgncmod-generic-import.dylib`gnc_import_TransInfo_new + 72
    frame #7: 0x000000010000d207 test-import-backend`ImportBackendTest_CreateTransInfo_Test::TestBody() + 1047

The way around that (and actually good practice in general for test programs) is build the test program from the source files that you're testing. It's pretty painful when the code you're trying to test has lots of dependent libraries, but it's what you have to do.

[gruberchr]

The way around that (and actually good practice in general for test programs) is build the test program from the source files that you're testing.

I agree, but this make things more complicated now. Function gnc_account_imap_find_account_bayes() is contained in Account.cpp. But also functions I actually wanted to use in the test, for example xaccMallocAccount() or xaccAccountSetName(), are contained in Account.cpp. So I can not simply exclude Account.cpp from the build process.
At the moment I have no idea how to deal with that.

[jralls]

No, absolutely not. A double-quoted string literal is of type const char* and it's instantiated in the code segment. std::string objects must be declared as such or come from a function returning it.

[jralls]

That doesn't say to put EXPECT_CALL in a fixture. In fact is says the opposite: A good design will use ON_CALL in the fixture to define the mock's behavior and EXPECT_CALL in the TEST_F to test that the code under test does in fact exercise that behavior.

[jralls]

It's a requirement of basic C++. Any object that allocates should be created in the fixture's setup and freed in the fixture's teardown because those functions are guaranteed to run even if the test throws an exception, whereas deallocation in the test function might not run if the function exits for some reason before the resources are freed. That's different from C test frameworks like GLib's because only a crash will prevent the function from going all the way through.

[jralls]

Inserting tests into spaghetti is hard, to the point that Michael Feathers wrote a well-regarded and very thick book on the topic.

The least invasive approach would be to mock all of the account functions you need or to work out how to test with the existing map functions. One thing Feathers suggests is refactoring functions to reduce interdependency; for example instead of branching on use_bayes, matchmap_find_destination could take the find_account function as an argument to enable you to insert the mock function. There's a little more to it, of course, because you're not calling matchmap_find_desitination directly in your test.

[gruberchr]

Not good prospects. In this case I would choose your first proposal and try to mock all of the account functions. Refactoring existing GnuCash code I would leave to you core developers at the moment. And testing import backend indirectly by observing the behavior of the find_account funtions seems quite inexact to mee.

[gruberchr]

I've given it some more thought. What do you think about shifting all functions from Account.cpp related to GncImportMatchMap into a new source code file? These are all gnc_account_imap_XXX() functions and their subfunctions. This way it would be quite easy to mock these functions without having to mock accounting functions as well. And the separation itself seems quite meaningful as well.

What do you think?
If you agree, I would prepare this. Would you like to open a new PR for this extra topic? And what filename would you choose for the new source code file?

[jralls]

Well, if you're testing the import map functionality then it makes more sense to mock the accounting functions and to use the actual import map functions.

The argument for having the gnc_account_imap functions in Account.cpp is that they modify the Account object by writing to its KVP slots. When Account gets rewritten as a C++ class then KVP will only be accessible to Account class member functions and their implementation will belong in Account.cpp (or maybe Account-impl.cpp if we decide that a pimpl design makes sense). What would be the justification other than possible convenience in testing the gnc_account_import_map functions in having those functions implemented in a different file?

I'd name such a file Account-import-map.[ch]pp so that it sorts near Account.cpp in the directory listing and to make clear that it's part of Account's implementation.

I'm also a bit skeptical that it would help you much. The new file will still have to be compiled into libgncmod-engine and the compile-time dependencies will still be in the image. You'll likely wind up creating a second libgncmod-engine-test with everything but Account-import-map.cpp compiled into it.

[gruberchr]

Well, if you're testing the import map functionality then it makes more sense to mock the accounting functions and to use the actual import map functions.

I'm not sure, if I understand that correctly. The intention of the test-import-backend is to test the import backend. It should not test the gnc_account_imap functions. Is that, what you mean with "import map functionality"? The gnc_account_imap functions are tested in the test-import-map. In the test-import-backend I want to mock the gnc_account_imap_find_account functions to test the function matchmap_find_destination() from import-backend.c.

The argument for having the gnc_account_imap functions in Account.cpp is that they modify the Account object by writing to its KVP slots.

Ok, I understand. Makes sense.

I'm also a bit skeptical that it would help you much. [...] You'll likely wind up creating a second libgncmod-engine-test with everything but Account-import-map.cpp compiled into it.

You're right. I've seen that problem now as well. Therefore I'll follow your advise and mock Account.cpp completely now. Would you also recommend to mock Transaction.c and Split.c from libgncmod-engine for that test?

[jralls]

You don't need a main(). Googletest takes care of that.

[gruberchr]

That's correct if only testing::InitGoogleTest() and RUN_ALL_TESTS() is called. Here I additionally wanted to initialize QOF and call cashobjects_register().

When I'll mock libgncmod-engine completely now, this will not be necessary anymore, I guess, and main() function can be omitted.

[jralls]

You're building it with gnc_add_test() in CMakeLists.txt. That will set up the program for you and call those functions. Having your own main should fail to compile because you're not allowed to have two.
GoogleTest provides a Global SetUp and TearDown facility where you can do things like call qof_init and cashobjects_register. But calling either of those gets you back to linking libgncmod-engine.

[gruberchr]

Ok, good to know.

[jralls]

You need to mock the functions that the function being tested calls. E.g. if you're testing gnc_import_TransInfo_new it calls xaccTransGetSplit so you need to mock that. It also calls matchmap_find_destination and that calls xaccTransGetDescription so you need to mock that too. I trust that you get the idea: Mock only what you need and write the mocks in such a way that they check that the caller is doing what it's supposed to. If GoogleMock actually helps with that when the code being tested is C rather than C++, great, otherwise you'll need to do it yourself.

[gruberchr]

I trust that you get the idea

Ok, I'll mock all required functions from libgncmod-engine completely now.

If GoogleMock actually helps with that when the code being tested is C rather than C++, great, otherwise you'll need to do it yourself.

My first idea behind using GoogleTest/GoogleMock although it's not C++ code to be tested/mocked, was that you don't need to rewrite the test code completely in the future, when the code to be tested is ported to C++. Would you agree to using GoogleMock instead of another C mocking framework in this situation.

[jralls]

Like I said, use GMock if you can make it work, otherwise you'll have to write the mocks by hand. We don't want to add a dependency on a C mocking framework.

[gruberchr]

I finally made it after some time. I have (with a few small exceptions) completely replaced libgncmod-engine.so by mockups. The branch is rebased on current maint.

I have identified two kinds of mockups so far:

1. Mockups for functions operating on C structs as objects
2. Mockups for GObjects

The first kind of mockups is still quite easy to realize. The second kind, GObjects, was a real challenge and that's what I have been working on for the longest time. But although I had the same impression as you at the beginning that GoogleMock is not desigend for mockups of GObjects, I have changed my opinion in the meantime (see below). The GObject framework would in theory be even better suited for mockups using GoogleMock than simple functions on C structs, if the concept of GObject interfaces is used. But as far as I have seen, these are not used in GnuCash. The main problem is, that the GObject framework is quite complicated.

Here are my approaches now. To mock a function operating on simple C structs as objects (first kind) I did the following:

1. Define a mock class, which is derived from the C struct
2. Define a corresponding gmock method of the mock class for each function, which should be mocked (replaced)
3. Replace each function by a mock function (C function), which casts the pointer on the C struct it's operating on to a pointer on the derived mock class and call the corresponding mock method using this pointer

Now to the second kind of mockups. For this I had to get familiar with the GObject framework documentation at first. I had not developed with the GObject framework before. I even looked at the GObject source code to understand how GObjects are implemented. I also looked for instructions and practical examples for writing C++ bindings on GObjects. But everything I have discovered so far has not helped me.

Small fun fact: When I typed the terms "gmock" and "GObject" into my search engine, I had this GnuCash Wiki link as third hit in the hit list.

Basically the approach for GObjects is an extension of the first approach. One difference is that GObjects must be allocated and initialized using the function g_object_new() and freed using the function g_object_unref(). To realize a basic mockup for a GObject I have chosen the following approach

1. Define a mock class, which is derived from the GObject type instance (C struct) to be mocked
2. Define a corresponding gmock method of the mock class for each GObject method
3. Define a new GObject subtype derived from the same base type as the GObject type to be mocked using the mock class as GObject type instance
4. Overload new and delete operators of the mock class, so that these use functions g_object_new() and g_object_unref() to allocate and free memory for the mock class instance
5. Make delete operator protected to force mock class instances to be allocated on heap (see e.g. https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Requiring_or_Prohibiting_Heap-based_Objects ), since GObjects can only be instantiated on the heap with g_object_new()
6. Override each GObject method by a new method, which casts the pointer on the GObject type instance to a pointer on the derived mock class and call the corresponding mock method using this pointer

This is a first draft and not a finished concept. I am aware that this does not represent all aspects of GObject framework by far. For instance at least the concept of object properties, which are used in many places in GnuCash code, is missing. But for my test application these are not needed yet. And I think the approach is extensible in this respect.

And finally some general thoughts about the use of GoogleMock for GnuCash code. It is correct that GMock is primarily intended for mockups based on interfaces (interface classes). And GnuCash code does not use this design so far. But this is actually not a requirement, it only concerns the way of connecting mock objects to existing code. Using interfaces and virtual methods, real objects can be replaced by mock objects dynamically at runtime. Without virtual methods, on the other hand, it must already be determined at compile time whether a mock object or the real object is to be used. This is described analogously in the section "Mocking nonvirtual methods" in the docs. As far as I can evaluate so far, there is no restriction concerning the usable functionalities of GoogleMock when mocking nonvirtual methods, only the build process is different. Another question is whether GMock is suitable to mock C code. The answer to this question depends in my opinion on whether it is possible to write a suitable C++ binding for the C code. If the C code follows an object-oriented design, one important prerequisite is fullfilled for instance. And so far it seems to be possible to use GoogleMock for GnuCash code.

[gruberchr]

Hi @jralls, do you have any feedback for me regarding my proposal on using GMock for GnuCash?

[jralls]

@gruberchr Sorry, I've been busy on other work.

Basically the approach for GObjects is an extension of the first approach. One difference is that GObjects must be allocated and initialized using the function g_object_new() and freed using the function g_object_unref()

That's true for GObjects that are correctly written to use the GObject memory management facilities. It's unfortunately not true of anything in the GnuCash engine.

Another question is whether GMock is suitable to mock C code. The answer to this question depends in my opinion on whether it is possible to write a suitable C++ binding for the C code.

That doesn't make sense. C functions are directly callable from C++. Do you mean that it depends on whether it's possible to coerce GMock to create a function with extern "C" linkage?

[gruberchr]

That's true for GObjects that are correctly written to use the GObject memory management facilities. It's unfortunately not true of anything in the GnuCash engine.

Ok, not good and might be a problem. But does this really matter, if the real implementation shouldn't be used anyway, but should be replaced by a mock up?

Do you mean that it depends on whether it's possible to coerce GMock to create a function with extern "C" linkage?

No, the focus of my statement is on "suitable". It doesn't make sense to use GoogleMock, if for instance the C code to be mocked does even not follow an object-oriented design. It's a basic concept of GoogleMock to mock classes. Of course you even could create a completely new GMock class for mocking plain C functions. But is it meaningful to use GoogleMock in this case?

[jralls]

That's true for GObjects that are correctly written to use the GObject memory management facilities. It's unfortunately not true of anything in the GnuCash engine.

Ok, not good and might be a problem. But does this really matter, if the real implementation shouldn't be used anyway, but should be replaced by a mock up?

I have to back off and revise that to "many things", because some do fake it as you found with Account, Transaction, and Split: There's no ref counting but the "xaccFooDestroy" functions unref the GObject instance. It seems to be good enough.

No, the focus of my statement is on "suitable". It doesn't make sense to use GoogleMock, if for instance the C code to be mocked does even not follow an object-oriented design. It's a basic concept of GoogleMock to mock classes. Of course you even could create a completely new GMock class for mocking plain C functions. But is it meaningful to use GoogleMock in this case?

That's the harder question. You've made it work for a couple of functions but it took a ton of code and a lot of hours to get there. GObject classes sort of behave like real classes through a lot of brute force and you've managed to duplicate enough of that brute force to make GMock work well enough for a couple of simple wiggle tests. Is it worth it? Do you think that you've built a base on which you can easily test the more complicated functions?

[jralls]

Overall a remarkable manipulation. It needs some more comments explaining what's going on so that one doesn't have to spend a lot of time figuring it out when one wants to work on it.

I do wonder how well this will expand into more complete testing of the import core and made worthwhile to use elsewhere in GnuCash.

Also it needs to be rejiggered to merge into master. It's too late for maint at this point, and besides you've used C++14 delete and maint is specced for C++11.

[jralls]

That's not a good reason to use std::list. The whole point of mocking is to ignore the mocked class's implementation details... and if you ever have to pass a GList* it won't help to have a std::list.

Does GMock require that its generated findAccountBayes take a std::vector<std::string>? If not you might as well make it a std::vector<const char*> and save the std::string allocation and construction.

[gruberchr]

That's not a good reason to use std::list.

Ok, I see.

Does GMock require that its generated findAccountBayes take a std::vector<std::string>?

No, it doesn't. GoogleMocks string matchers accept both. I'll change it to std::vector<const char*>, makes sense.

[gruberchr]

Done in PR #738

[jralls]

That's not quite true. It and gnc_mock_account_class_init are called by G_DEFINE_TYPE. You could as easily do the work here as in the C++ ctor... and note that for GObject foo_init and foo_class_init the constructors; you should say "MockAccount's C++ constructor".

[gruberchr]

Of course these two functions are called during GObject instantiation. The comment should say, that the C++ ctor is used for instance initialization instead of this function.

Is it clear enough, when I change "MockAccount's constructor" to "MockAccount's C++ constructor" then.

[jralls]

I think so.

[gruberchr]

Done in PR #738

[jralls]

Is there a reason to be passing the vectors around by value instead of by reference?

[gruberchr]

Seems to work. I'll change it.

[gruberchr]

Done in PR #738

[jralls]

ISTM you should use EXPECT_TRUE() or maybe ASSERT_TRUE instead of g_return_if_fail. The latter will just write a message to std::cerr instead of causing the test to fail.

[gruberchr]

Ok, I'll change it

[gruberchr]

Done in PR #738

[jralls]

also #include <glib-object.h>

[gruberchr]

Ok, I'll add this

[gruberchr]

Done in PR #738

[jralls]

Avoid the singleton pattern and lots of others..

There's no benefit to this being a singleton or even single-instance.

[gruberchr]

The underlying implementation of the preferences backend follows a singleton pattern by using global pointer prefsbackend. Therefore the mock up has to use it as well.

[jralls]

Uh, that's not a good reason: The whole point of a mock is to avoid implementation details, and the global variable is just that: An implementation detail.

A better answer would be something along the lines of "gnc_prefs_foo needs some way to access the preferences and since it doesn't have a preferences parameter it needs something internal and a singleton is an easy way to do that." Except that you're not using the singleton for that, you're using a global variable for it and doing so in a poorly thought out way.

The global belongs to gmock-gnc-prefs.c so it could be static with a setter . MockPrefsBackend can be an ordinary class with a default constructor and destructor. Let there be static MockPrefsBackend* prefsbe and gnc_prefs_set_backend(MockPrefsBackend*). Your fixture code would do

   SetUp ()
   {
          gnc_prefs_set_backend(&m_prefs);
          // ...
    }
   MockPrefsBackend m_prefs;

[gruberchr]

Oh yes, I haven't thought again about this implementation. I used the singleton pattern in my first approach, where it was really necessary, since I linked libgnc-core-utils.so into the test application, which contains gnc-prefs.c and defined global pointer prefsbackend.

Now this is not necessary anymore and could be changed as you suggested.

[gruberchr]

Done in PR #738

[jralls]

You mean without calling qof_instance_set_dirty().

[gruberchr]

Oh yes, right.

[gruberchr]

Changed in PR #738

[jralls]

Say "Note:" or "N.B." (nota bene, latin for note well) instead of "hint:".

[gruberchr]

ok

[gruberchr]

Done in PR #738

[jralls]

Using the terms defined in GMock docs this is a fake-qofquery rather than a mock-qofquery, and gmock doesn't have anything to do with it.

[gruberchr]

Ok, I'll check that. Do you have a proposal for a naming convention?

[jralls]

fake-qofquery.h?

[gruberchr]

Done in PR #738

[jralls]

They're fakes/replacements, not mocks, and the comment should say why they're needed, e.g. being defined in a library we don't want to link to.

[gruberchr]

Correct

[jralls]

The point is that the comment needs to say that.

[gruberchr]

Changed in PR #738

[gruberchr]

GObject classes sort of behave like real classes through a lot of brute force and you've managed to duplicate enough of that brute force to make GMock work well enough for a couple of simple wiggle tests. Is it worth it?

What do you think? It's surely not worth it for only a handful of tests. If it helps to write a lot of more tests, which are not possible to write up to now with reasonable effort, it makes sense I think.

Do you think that you've built a base on which you can easily test the more complicated functions?

I can not answer this question. I don't know the GnuCash code as good as you. But I hope so.

Also it needs to be rejiggered to merge into master. It's too late for maint at this point.

I was expecting this.

The GoogleMock Docs recommend keeping the mocks and the originals together, both so that the mocks can be used easily from any tests that need them and to make it easier to keep the mocks in sync with the mocked when the latter changes.

I fully agree.

When I started writing this test, I hadn't thought, that it gets that complicated at the end. But all the work has been done and it would be a pity, if all the hours were in vain. As you wrote as well, it's a proof-of-concept for introducing mockups into GnuCash test code.

Therefore I'm asking you know to decide how to proceed. I guess it would be helpful, if you prepare a proposal, where to put all the mock up code and what to change based on my first draft.

[jralls]

@gruberchr sorry for the delay, I wanted to get @gjanssens to have a look and consult with him about where to put the mocks. We decided that the cleanest location is a mocks subdirectory under the one with the objects being mocked, so in this case libgnucash/engine/mocks.

I've made the necessary changes in the merge commit and merged it into master. Thanks!

[gruberchr]

Thanks @jralls for merging. As I have seen, you did not touch the code, but only moved the files in the merge commit. What about your open remarks above? Change it now or leave it?

[jralls]

Please answer the questions and proceed to fix the rest. I merged because none of them are real show-stoppers--this is after all test code--and you seemed to be waiting for something to happen with regards to moving branches.

[gruberchr]

Hi @jralls can you or @gjanssens have a look at PR #738?

It doesn't influence the GnuCash 4.0 release.

[jralls]

@gruberchr You can just ping on the other PR. I'll pick up reviewing the open PRs in a week or two, I've been focussing the last month on getting 4.0 ready for release so I have a pile of non-GnuCash work that needs my attention.