Support for testing framework #129

lobis · 2022-02-04T18:21:21Z

Related to #125.

The goal of this PR is to clean the CMake build system wherever possible (it is really complex, it could probably be simplified) and also modify it to support unit testing via ctest. Also I would like to install external dependencies via cmake instead of just copying them into the framework (such as tinyxml) but I won't change tinyxml since its been this way forever, if anything we should probably remove this dependency altogether and replace it by the ROOT xml library which can do what we want.

Testing is enabled via the -DTEST=ON flag, disabled by default.

Tests can be run via ´ctest´ from the build directory.

I will write the boilerplate for testing in all modules. (#163).

Some information about running tests can be found at rest-for-physics/rest-for-physics.github.io@43a1e66.

jgalan · 2022-02-07T10:51:31Z

As we get more and more activity on the repositories we should be more and more explicit on describing the intention of the updates at a specific PR. Please, give details on the problem and the enhancements that will bring this PR, including the reasons that triggered it. Those habits will be good to incentive discussions on the different contributions.

# Conflicts: # CMakeLists.txt # cmake/MacroRootDict.cmake # source/framework/CMakeLists.txt

lobis · 2022-02-08T11:44:11Z

As we get more and more activity on the repositories we should be more and more explicit on describing the intention of the updates at a specific PR. Please, give details on the problem and the enhancements that will bring this PR, including the reasons that triggered it. Those habits will be good to incentive discussions on the different contributions.

That is why I made the PR a draft, so I could fill the information as I go, but I agree I should have been a bit more detailed. Thanks!

lobis · 2022-02-08T11:54:31Z

Currently there is a problem I don't know how to fix, perhaps @nkx111 or @jgalan can give me a hand. It has to do with the startup.cpp script.

Currently I cannot compile the tests due to an error at the end of compilation:

sh: 1: rest-config: not found
CMake Error at /usr/share/cmake-3.16/Modules/GoogleTestAddTests.cmake:40 (message):
  Error running test executable.

    Path: '/tmp/tmp.1ggMShb7jT/cmake-build-dockerdev/source/framework/test/testFramework'
    Result: Child aborted
    Output:
      REST ERROR!! Lacking system env "REST_PATH"! Cannot start!
      You need to source "thisREST.sh" first

To get the error just compile REST with the new TEST flag on: cmake -DTEST=ON ...

I am not sure how/why a script is being run at compilation but for the all the tests targets I would like to avoid it, but don't know how to do this without probably breaking something since I don't understand this part of the code. This is blocking testing right now :(

I think the implementation of the tests is good and I can't think of a modification that would avoid this so we probably need to modify startup.cpp or some cmake section that is "linking" this?

Thanks!

nkx111 · 2022-02-08T12:52:59Z

Currently there is a problem I don't know how to fix, perhaps @nkx111 or @jgalan can give me a hand. It has to do with the startup.cpp script.

Currently I cannot compile the tests due to an error at the end of compilation:
sh: 1: rest-config: not found
CMake Error at /usr/share/cmake-3.16/Modules/GoogleTestAddTests.cmake:40 (message):
  Error running test executable.

    Path: '/tmp/tmp.1ggMShb7jT/cmake-build-dockerdev/source/framework/test/testFramework'
    Result: Child aborted
    Output:
      REST ERROR!! Lacking system env "REST_PATH"! Cannot start!
      You need to source "thisREST.sh" first
To get the error just compile REST with the new TEST flag on: cmake -DTEST=ON ...

I am not sure how/why a script is being run at compilation but for the all the tests targets I would like to avoid it, but don't know how to do this without probably breaking something since I don't understand this part of the code. This is blocking testing right now :(

I think the implementation of the tests is good and I can't think of a modification that would avoid this so we probably need to modify startup.cpp or some cmake section that is "linking" this?

Thanks!

It seems that the new ExampleTest.cxx is being executed and raised this error. The part of error code is from startup.cpp and will run as long as the framework library is linked.

I suggest not to avoid linking it. We can try adding those envs to gtest system.

lobis · 2022-02-08T13:13:44Z

Currently there is a problem I don't know how to fix, perhaps @nkx111 or @jgalan can give me a hand. It has to do with the startup.cpp script.
Currently I cannot compile the tests due to an error at the end of compilation:
sh: 1: rest-config: not found
CMake Error at /usr/share/cmake-3.16/Modules/GoogleTestAddTests.cmake:40 (message):
  Error running test executable.

    Path: '/tmp/tmp.1ggMShb7jT/cmake-build-dockerdev/source/framework/test/testFramework'
    Result: Child aborted
    Output:
      REST ERROR!! Lacking system env "REST_PATH"! Cannot start!
      You need to source "thisREST.sh" first
To get the error just compile REST with the new TEST flag on: cmake -DTEST=ON ...
I am not sure how/why a script is being run at compilation but for the all the tests targets I would like to avoid it, but don't know how to do this without probably breaking something since I don't understand this part of the code. This is blocking testing right now :(
I think the implementation of the tests is good and I can't think of a modification that would avoid this so we probably need to modify startup.cpp or some cmake section that is "linking" this?
Thanks!
It seems that the new ExampleTest.cxx is being executed and raised this error. The part of error code is from startup.cpp and will run as long as the framework library is linked.

I suggest not to avoid linking it. We can try adding those envs to gtest system.

but how is it being executed on the build? How could we add this env to the gtest in a simple way? are you sure running some code when linking to REST library is a good practise? seems it can cause a lot of trouble.

lobis · 2022-02-08T13:30:42Z

something very weird happened with this branch I cannot fix the bug

jgalan · 2022-02-08T20:24:23Z

Also I would like to install external dependencies via cmake instead of just copying them into the framework (such as tinyxml)

In principle, there is no problem to do that with the present cmake system, if tinyxml would be installed as a system library it would just pick up it. I believe @nkx introduced some modifications to the tinyxml library, so thats why the library is copied into the repository. Not because it is not possible to link external libraries.

…using problem)

CMakeLists.txt

juanangp · 2022-03-24T17:06:33Z

Perhaps you should mention somewhere that the support for old Garfield versions have been removed, since you removed FindGarfieldOld, therefore is related with rest-for-physics/detectorlib#41

This reverts commit e58bdf4.

lobis · 2022-03-25T11:18:08Z

Perhaps you should mention somewhere that the support for old Garfield versions have been removed, since you removed FindGarfieldOld, therefore is related with rest-for-physics/detectorlib#41

I decided to keep this file and remove it in a more appropriate PR.

jgalan · 2022-03-26T19:42:33Z

source/framework/test/files/metadata.rml

@@ -0,0 +1 @@
+<metadata  p1="75"  p2="12.32"  p3="Aloha" />


It does not keep the convention that the xml header is the name of the c++ metadata class, which is metadataTestClass.

jgalan · 2022-03-26T19:43:42Z

source/framework/test/src/FrameworkCore.cxx

+    EXPECT_TRUE(fs::exists(BASIC_TRESTMETADATA_RML));
+
+    // Create new TRestMetadata class
+    class metadataTestClass: public TRestMetadata{


Any REST class should be starting by TRest

Any REST class should be starting by TRest

but this is not a class, its just a cpp file

well, it is a class, as it is stated in front of it name, I agree we could make an exception inside tests, but I thought it was not possible that the XML header and the class name would be different, it is this possible? @nkx?

Okay now I see what you mean. but this is not a class definition, the same test file could contain many class definitions, it should not begin by TRest imho.

This was introduced by @DavidDiezIb and I think its a great idea to test a class such as TRestMetadata.

I don't mind the naming convention, we can change it if you prefer. Just I thought it was better not to use REST naming convenctions to avoid confussion with any real REST classes.
This is a fake one created to be able to test GetParameter function from TRestMetadata. It is a pure abstract class so we can't call it, we can only inherit from it.

Related to the name of the metadata section, I can modify it to coincide with name of the class, maybe is easier to read. But meta.LoadConfigFromFile(BASIC_TRESTMETADATA_RML, "metadata"); is telling where has to look for the parameters to initialize metadataTestClass (the XML section called "metadata") so doesn't seem to have any problem.

This was introduced by @DavidDiezIb and I think its a great idea to test a class such as TRestMetadata.

I am not debating that

Yes, that's right, maybe we should remove the option to select the section by hand in LoadFromConfigFile. I don't know if this is useful somewhere.

Yes, I didn't know that was possible, we never use it that way, isn't @nkx?

Int_t TRestMetadata::LoadConfigFromFile(string cfgFileName, string sectionName) { fConfigFileName = cfgFileName; if (TRestTools::fileExists(fConfigFileName)) { if (sectionName == "") { sectionName = this->ClassName(); }

So probably we should remove the second argument of that method, and just define a local variable sectionName=this->ClassName()?

Exactly, I agree.

However I am a bit surprised, I thought the second argument was the given name, so that if I got two TRestMyMetadata definitions I am able to select the one I am interested in:

<TRestMyMetadata name="md1" /> <TRestMyMetadata name="md2" />

So that the constructor would pick up the right definition using:

TRestMyMetadata *md = new TRestMyMetadata("file.rml", "md1" ); md->LoadConfigFromFile( "file.rml", "md1");

At least thats how I am using it inside for example TRestAxionMagneticField and it works.

I guess it is because either option is posible as from the definition NameOrDeclare at:

TiXmlElement* TRestMetadata::GetElementFromFile(std::string cfgFileName, std::string NameOrDecalre)

But why do we need Declare @nkx?

lobis · 2022-03-27T11:00:27Z

I think we should not make any more changes to this branch from this point and merge into master unless someone has any objection @rest-for-physics/core_dev @jgalan . More tests can be added in other PRs.

…nto cmake-ctest

jgalan · 2022-03-28T08:28:44Z

The upgrade at #165 should allow now to include a test using directly TRestRun.

update docker image

a1e680f

lobis requested review from jgalan, juanangp and nkx111 February 4, 2022 18:21

lobis self-assigned this Feb 4, 2022

Started refactoring

f558872

lobis added 2 commits February 8, 2022 12:35

Added installation of external dependencies via cmake, added testing PoC

62f5a7a

Merge commit '62f5a7a8d0da762a142a74c22db83da127c65b45' into cmake-ctest

300f0fb

# Conflicts: # CMakeLists.txt # cmake/MacroRootDict.cmake # source/framework/CMakeLists.txt

fixed external deps

cd744cb

fixed bad image reference

c7c10f5

lobis added the enhancement New feature or request label Feb 8, 2022

This was linked to issues Feb 8, 2022

Introduce a testing framework #109

Closed

Refactor CMake build #125

Closed

Merge remote-tracking branch 'origin/cmake-ctest' into cmake-ctest

4b9abf2

lobis added 7 commits February 9, 2022 09:54

Merge branch 'master' into cmake-ctest

56266fa

making cmake-ctest match master

18cb64a

updated image

a672208

updated cmake

6446b82

formatting cmake, added testing macro

d85d550

added test example

9701337

Added quick fix for testing, TODO: fix startup.cpp (or whatever is ca…

e2660bd

…using problem)

jgalan reviewed Feb 9, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Merge branch 'master' into cmake-ctest

ea9dd48

Revert "cmake - removed old FindGarfieldOld.cmake"

dc20190

This reverts commit e58bdf4.

lobis requested review from DavidDiezIb, glorialuzon and jgalan March 25, 2022 11:19

lobis added 4 commits March 25, 2022 14:54

Testing - better structure for test file

8902794

Testing - updated executable name

ddf1a6d

Removed old testing directory as it is now obsolete

8acb468

Updated testing cmake to reduce code replication

2f3b4b7

This was referenced Mar 25, 2022

Create testing boilerplate for all libraries / packages #163

Closed

TRestRun::TRestRun(string) constructor supports both root file and rml file initialization. #165

Merged

DavidDiezIb added 2 commits March 26, 2022 20:34

New Test for TRestMetadata

b7ca972

Remove commented line

b3c32b4

jgalan reviewed Mar 26, 2022

View reviewed changes

lobis added 2 commits March 27, 2022 12:04

Testing - formatting

d06a89f

Testing - refactored testings by @DavidDiezIb

4104604

lobis requested a review from jgalan March 27, 2022 10:08

lobis added 2 commits March 27, 2022 12:27

cmake/Testing.cmake - removed msg

337138a

Testing - fixed error in test

b6efb43

jgalan approved these changes Mar 27, 2022

View reviewed changes

lobis mentioned this pull request Mar 28, 2022

Remove early return on startup.cpp when testing is enabled #169

Open

Merge branch 'cmake-ctest' of github.com:rest-for-physics/framework i…

bf592d3

…nto cmake-ctest

juanangp approved these changes Mar 28, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into cmake-ctest

6f85645

lobis merged commit b05be52 into master Mar 28, 2022

lobis deleted the cmake-ctest branch April 26, 2022 11:51

		@@ -0,0 +1 @@
		<metadata p1="75" p2="12.32" p3="Aloha" /> No newline at end of file

Support for testing framework #129

Support for testing framework #129

Uh oh!

Conversation

lobis commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgalan commented Feb 7, 2022

Uh oh!

lobis commented Feb 8, 2022

Uh oh!

lobis commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkx111 commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lobis commented Feb 8, 2022

Uh oh!

lobis commented Feb 8, 2022

Uh oh!

jgalan commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

juanangp commented Mar 24, 2022

Uh oh!

lobis commented Mar 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgalan Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobis Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobis commented Mar 27, 2022

Uh oh!

jgalan commented Mar 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lobis commented Feb 4, 2022 •

edited

Loading

lobis commented Feb 8, 2022 •

edited

Loading

nkx111 commented Feb 8, 2022 •

edited

Loading

jgalan commented Feb 8, 2022 •

edited

Loading

jgalan Mar 26, 2022 •

edited

Loading

lobis Mar 26, 2022 •

edited

Loading