[all] Refactor SofaTest to cut dependencies

[damienmarchal]

This is a variation of the proposal done by @fjourdes in #455 where I allow me a bit more refactoring.

Here is what the PR does:

• implement basic test class in sofa/helper/testing/ BaseTest & NumericTest so that all framework tests can use them.
  • in SofaSimulationGraph/testing add a BaseSimulationTest class that implement facilities for tests that needs scene manipulation & loading.
  • udpates SofaTest to use the previously defined classes but still behaving as they used to in the past.
  • updates the framework_test to use BaseTest or NumericTest in place of Sofa_test or ::testing
  • moves the tests in framework to framework_extra when they are manipulating a simulation (thus requires BaseSimulationTest instead of BaseTest)
  • updates the tests in simulation_test so that they don't use SofaTest anymore.
  • moves SimpleApi from SceneCreator to SofaSimulationGraph & fix a bug.
  • creates a plugin in a the application/package directory so that in our tests we can load all the common sofa component with a single RequirePlugin("SofaAllCommonComponents")

The consequences are:

• SofaTest is no more needed in framework & simulation
• we can now progressively make the other tests to rely on BaseTest, BaseSimulation and SimpleAPI that are dependency free instead of SofaTest/SceneCreator.

Feel free to provide feedback/comment.

---

This PR:

• [x] builds with SUCCESS for all platforms on the CI.
• [x] does not generate new warnings.
• [x] does not generate new unit test failures.
• [x] does not generate new scene test failures.
• [x] does not break API compatibility.
• [x] is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

[damienmarchal]

One possible next-step could be to connect the BaseSimulationTest with the simpleapi work discussed with @maxime-tournier. So that we would have a fully loosed coupling equivalent to scenecreator and sofatest with good looking syntax.

[fjourdes]

I believe whatever the approach taken it should allow to keep things pretty clear between

• unit tests, ie tests that hardly depend on initializing a scene. In these tests you hardly need a concrete instance of node or simulation. If you need this it may be nice to have a class to derive from which does it for you, but then again it should stay optional, because you should really think again about how you designed your component if you need a full fledged simulation to actually make some basic testing on your component. No matter what, these tests should be executed at each build (even for devs) and must pass. Failing in this kind of tests must be equal to a compilation error. In that respect there is something that I am reluctant to allow for this type of tests is a lose coupling approach where you instantiate components using keys from the object factory. You may end up writing tests that in fact do not compile from a scratch build because this allows you to not take care about the dependencies between the libs. That is to say a test may instantiate a component that is not available yet, because it belongs to a library which is deeper in the build dependency tree at the time the unit test is built and executed during the compilation.

• Functional tests, that require a working scene with probably more components than the one actually tested. These tests can be longer and therefore should not be mandatory during the compilation. They will be run only in ci machines, but can be also easily executed by any developers on its machine mostly to debug what is the nature of a new regression in these kind of tests. From our experience at InSimo these kinds of tests regress quite often during the development of a new feature, because you make some optimisation in the code, and you get some epsilon difference, or because you change the approach entirely and the scene is no longer relevant for you and should be changed. So you probably do not want too many of these functional tests, but there should be enough so that it is representative of the overall kind of simulations you want to support. But anyway it is still nice to have a record of the regressions that occur during the overall development, because you can then document better when you actually introduced changes in the code that are significant, which certainly is hard to track by unit tests only, unless you have a coverage of 100%, which is a target that in my opinion is a waste of energy to aim at, provided you have some functional tests also.

[damienmarchal]

Hello François (@fjourdes),

Thank for the long and interesting reply.

Your last comment rise an interesting question related to differentiating unittest to functionnal tests. Currently in Sofa we don't make any distinction between unit test and functionnal test but my thinking is that in general utility classes have unit tests while components have functional tests . Making the distinction would be indeed better so the question is how to get there from where we are a now.

Here is a proposal. In the current PR there is a BaseTest and BaseSimulationTest, this could actually be transformed without too much effort into UnitTest and FunctionalTest. The tests inheriting from UnitTest shouldn't use the factory in any way and shouldn't have external dependencies. The test in-heriting from FunctionalTest should use the factory and can make the assumption that specific plugins are loaded. We could event go one step further and emphasizing things by having two set of file, ones named MyClass_unittest.cpp and MyComponent_functionaltest.cpp and in our CMakeLists.txt compile the two set of tests separately with different build targets.

This would make things very clear, be compatible with your workflow where unit test are considered as compilation failure, be compatible with our workflow where functional tests are used because they allow to get decent code coverage and last point as it would use the loose coupling approach it would totally cut the dependency at compile time to SofaTest.

Small note, some Functional tests may rely on components implemented in plugins. I have not clear view if we should disable the test if the plugin is not there or if we should make it fails or if we should "disable" it at runtime.

EDIT: changes some sentences.

[guparan]

@fjourdes: your opinion on latest comment? I prefer this version as I think we should keep an interface to gtest.

[fjourdes]

See my comment in the remarks section of #487

I am really not sure about the current state of SofaGTestMain. It is hidden somewhere in the folder hierarchy (inside tools directory), and its existence seems to imply that you cannot write a unit test inside sofa without relying on it... which is absolutely not the case. See PR #471 Instead having a library which depends on gtest and SofaCore which

• factorize some initialisation methods when they are actually required in the test SetUp() method
• provide some useful abstractions when you want to test some important concepts of a component, like what SofaTest wanted to do, but without the dependency bloat. A potential non exhaustive list of abstactions such a library could provide
  • Mapping
  • linearisation,
  • applyJ applyJT equivalence
• ForceField
  • linearisation
  • addDForce addKToMatrix equivalence
  • addKToMatrix must give a SPD matrix (well actually in Sofa convention I think internal forcefields actually give a SND matrix but the idea remains...

Nothing of this testing API would require the initialisation of a simulation / node.

At this stage to be honest I am not even 100% sure we even need a library for that, maybe free methods are enough.

[fjourdes]

@guparan : however for example unit tests inside SofaCore / SofaDefaullttype / SofaHelper cannot rely on a Sofa specific testing abstraction layer written on top of gtest, if the guildeline is that a unit test executable can be run solely by linking against the library it is designed to test.

[damienmarchal]

Hello François,

Here is a summary of my current understanding of your comments. It seems that you want to: 1 - cut the dependency to SofaTest 2 - have a clear separation between unittest vs functionaltest 3 - cut the dependency to SofaFramework (ie not having the test inheriting from SofaFramework/BaseTest or something like that) for the unittests.

Initially this PR was only about -1- cutting the dependency to SofaTest. My thinking is that the SofaTest dependency is a major problem for everyone using the master branch of sofa because of the high impact on code coupling.

About -2- Following your remarks I'm in favor of improving a more clear separation between unit-test and functional test but as this is something new and maybe this should discussed that in the "issues" section of github instead of inside a specific PR.

About -3- cutting the dependency to SofaFramework. This is something that was never discussed before and worse than that, up to now the general consensus that, as we are in Sofa, we should inherit from a single BaseTest that install all the may be needed to do the test. A good reason for this scenario is nothing related to Scene/Simulation management but because BaseTest provides a default handler for EXPECT_MSG_EMIT/NOEMIT. Of course if your code is not using at all those elements, then you should feel free to use gtest.

As said in previous comment compared to the current Sofa code base I would be very happy to have -1- and -2- done.

It seems that -3- is not a big deal for people using the CMakelists.txt from Sofa but it may be of importance for your specific workflow in which, if I understand your previous answers, you integrate unittest as compilation failure. Maybe you can provide us more details on your workflow so that we can see the drawback of having the tests written in SofaFramework to be inheriting from a BaseTest (which is also defined in SofaFramework).

Damien.

[fjourdes]

@damienmarchal Sorry I did not pay attention that BaseTest reside in SofaHelper so that is fine, I thought your idea was to implement a new library dedicated to the abstraction api for testing with sofa, ie a kind of lightweight SofaTest library. I was not really aware that there was code inside SofaKernel libraries that directly depend on gtest, I thought it was still kept "optional" (ie that gtest has become mandatory on master for some time, wether or not you build and execute unit tests). We are still used to developping with the "idea" that gtest is optional, even though in reality there is no use case were we do not include it.

Coming back at this PR, to be honest I don't believe there is a functionality in the BaseTest classes (apart from the fact that it removes dependencies) that I would like to use, because mostly :

• we do not currently have a use for message api, or at least we do not have any idea about why it could be beneficial for us to start using it.
• I d like to discourage the use of one size fits all epsilon and comparison values for floating points arithmetic, since I prefer people to actually think about the quantities they want to compare. Comparing floats in a consistent fashion is harder than it looks, with the exception when you expect that the quantities you want to compare to be strictly equal. So having a Base test class which provides these comparison facilities is also not something I am particularly looking for.

There are some abstractions attempts in SofaTest that I find interesting ( linearisation tests for the most part ) but it got snowed under a dependency bloat.

However that being said, the only thing I'd like, is that if I implement a test just using the "vanilla" gtest library, it can be merged, built and run easily on the master. With the current state of things, integrating a new test in framework_test, requires you to build all sofa, and not just SofaCore as I would have expected. So in return you have to wait a loooong time before you can actually check that the functionality you want to integrate in master actually works as you expect.

[damienmarchal]

About this PR...please keep in mind that its target is to cut the dependency to SofaTest.

About the NumericTest base test class (I think this is the one François is refering to this one when he is saying "I would like to discourage the use of one size fits all epsilon and comparison values for floating points arithmetic, since I prefer people to actually think about the quantities they...") it is there because there was this functionallities in SofaTest and I didn't want to update all the tests in this PR. So I just move the features from SofaTest to SofaFramework. My strategy is to 1) cut the dependency with this PR, 2) update all the tests in modules small at a time so that they stop using SofaTest. And in a third step if NumericTest is really bad...then maybe someone will update the tests relying on it and remove the class :)

[bcarrez]

From what I understand, we have 2 different topics here.

1. cut dependencies to SofaTest, which I totally approve. This point reaches consensus I think. I don't see anything that can go against the merge of this PR in this topic

2. split unit tests and functionnal tests This is a different topic and should be discussed in a separate issue. Anyway my 2 cents about this:

   • obviously this is a good idea. I cannot push forward enough this idea.
   • just keep in mind that these unit-tests should remain FAST to run, since they will be run at each build. (ie. the filemonitor tests, for example, should be refactored to spend less than 10 seconds to run otherwise they will soon become a pain)

To resume my point of view:

1. merge this PR first
2. communicate around the fact that tests should not rely on SofaTest anymore (when possible) ==> why not log a warning or "deprecated" message when using SofaTest utilities ?
3. progressively cut unit tests from SofaTest, on-the-go

In parallel, open the "always run unit-tests" discussion on a separate topic.

[guparan]

My change is not a regression. Builds are now failing when unit tests crash for an unexpected reason (not due to the test itself). Sorry to kind of break your PR @damienmarchal :-/

[damienmarchal]

@guparan No problem guillaume and you are totally right with that. I spend the whole day yesterday to narrow the problem but cannot find it yet.

[damienmarchal]

@guparan & @fredroy I think I fixed the test failure problem on centos & ubuntu by removing the add_target_library( gtest ) in Sofa_test. Don't ask me why it remove the failure at release I don't know ;)

[damienmarchal]

[ci-build][build-scenes]

[damienmarchal]

[ci-build][with-scene-tests]

[damienmarchal]

Maxime (@maxime-tournier) I would appreciate to have your feedback on this PR.

[maxime-tournier]

Hi @damienmarchal, will look into it ASAP, but I don't have much time these days unfortunately :-/

[damienmarchal]

@maxime-tournier I would alreay be happy with a feedback on the approach taken to cut the depdencies. I'm asking because I know the topic interests you.

[damienmarchal]

@hugtalbot , @guparan and everyone else This PR

• have discussed with as much as possible persons,
• and is now one month old,
• succeed on all Tests on the CI since several days

I suggest to have a last look at it & merge it rapidly that we can start working on top of it to improve it and to remove all the dependencies to SofaTest.