Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for testing framework #129

Merged
merged 69 commits into from
Mar 28, 2022
Merged

Support for testing framework #129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
a1e680f
update docker image
lobis Feb 4, 2022
f558872
Started refactoring
lobis Feb 7, 2022
62f5a7a
Added installation of external dependencies via cmake, added testing PoC
lobis Feb 8, 2022
300f0fb
Merge commit '62f5a7a8d0da762a142a74c22db83da127c65b45' into cmake-ctest
lobis Feb 8, 2022
cd744cb
fixed external deps
lobis Feb 8, 2022
c7c10f5
fixed bad image reference
lobis Feb 8, 2022
4b9abf2
Merge remote-tracking branch 'origin/cmake-ctest' into cmake-ctest
lobis Feb 8, 2022
56266fa
Merge branch 'master' into cmake-ctest
lobis Feb 9, 2022
18cb64a
making cmake-ctest match master
lobis Feb 9, 2022
a672208
updated image
lobis Feb 9, 2022
6446b82
updated cmake
lobis Feb 9, 2022
d85d550
formatting cmake, added testing macro
lobis Feb 9, 2022
9701337
added test example
lobis Feb 9, 2022
e2660bd
Added quick fix for testing, TODO: fix startup.cpp (or whatever is ca…
lobis Feb 9, 2022
ea9dd48
Merge branch 'master' into cmake-ctest
lobis Mar 8, 2022
7a9bc58
CMakeLists.txt - Updated website to GH pages (https://github.com/rest…
lobis Mar 23, 2022
61876c8
Merge remote-tracking branch 'origin/master' into cmake-ctest
lobis Mar 23, 2022
f177abc
external/CMakeLists.txt - Added more comments
lobis Mar 23, 2022
3e82b09
CMakeLists.txt - Removed optional settings from `PROJECT`, for some r…
lobis Mar 23, 2022
3c0d4f3
CMakeLists.txt - Bumped minimum `cmake` version to 3.16 to prevent is…
lobis Mar 23, 2022
7de333e
framework/test - renamed TRestRun test
lobis Mar 23, 2022
3d33f1c
Framework/test - Added basic test rml for TRestRun, using filesystem …
lobis Mar 23, 2022
9d1a9d0
framework/test - added test to initialize TRestRun from config file
lobis Mar 23, 2022
dd44a96
.gitlab-ci.yml - cleaned file, removed comments and unnecessary lines
lobis Mar 23, 2022
0e8c7ef
framework/test - skip test until fix
lobis Mar 23, 2022
4ff2fdf
.gitlab-ci.yml - now pipeline will first build with tests, run tests,…
lobis Mar 23, 2022
e663d12
.gitlab-ci.yml - running install AFTER tests
lobis Mar 23, 2022
e73f72a
.gitlab-ci.yml - formatting changes, replaced '.' by 'source'.
lobis Mar 23, 2022
8b8e5eb
.gitlab-ci.yml - small formatting changes
lobis Mar 23, 2022
572c99b
.gitlab-ci.yml - removed '\'
lobis Mar 23, 2022
10e03d0
.gitlab-ci.yml - added debug msg
lobis Mar 23, 2022
73534c1
.gitlab-ci.yml - added back rm .rest dir
lobis Mar 23, 2022
689db50
.gitlab-ci.yml - removed unecessary if
lobis Mar 23, 2022
2b35889
.gitlab-ci.yml - removed old commented code
lobis Mar 23, 2022
1f25358
.gitlab-ci.yml - fix typo
lobis Mar 23, 2022
a62f47d
.gitlab-ci.yml - added `printenv` for debugging
lobis Mar 23, 2022
af08e3d
.gitlab-ci.yml - added restRoot without alias
lobis Mar 23, 2022
2c4a202
.gitlab-ci.yml - added ls for $REST_PATH/bin
lobis Mar 23, 2022
846bb01
.gitlab-ci.yml - using explicit path
lobis Mar 23, 2022
9b7c217
.gitlab-ci.yml - removed some debugging commands
lobis Mar 23, 2022
ab03caf
.gitlab-ci.yml - added test output as artifact
lobis Mar 23, 2022
0d69a31
Merge remote-tracking branch 'origin/master' into cmake-ctest
lobis Mar 24, 2022
8ea86f2
TRestMetadata.h - minor comment updates
lobis Mar 24, 2022
182ae78
framework/test - Updated test thanks to feedback on https://github.co…
lobis Mar 24, 2022
a3aaa2c
framework/source/bin - formatting CMakeLists.txt
lobis Mar 24, 2022
f387dab
removed old `testing` directory
lobis Mar 24, 2022
edef6d9
framework/external - remove comment for unused external dependencies
lobis Mar 24, 2022
17ad3b9
CMakeLists.txt - removed unreachable external dependencies
lobis Mar 24, 2022
7cfa5d3
CMakeLists.txt - simplified cmake testing
lobis Mar 24, 2022
848f2da
CMakeLists.txt - reverted minimum version to 3.5, since it has some u…
lobis Mar 24, 2022
9555179
.gitlab-ci.yml - named artifacts
lobis Mar 24, 2022
45797b7
.gitlab-ci.yml - fix multiline command not working
lobis Mar 24, 2022
e58bdf4
cmake - removed old `FindGarfieldOld.cmake`
lobis Mar 24, 2022
4c962f0
.gitlab-ci.yml - pulling submodules twice, for the time being
lobis Mar 24, 2022
8bb3282
.gitlab-ci.yml - cmake now is multiline for better readability
lobis Mar 24, 2022
15d0f83
.gitlab-ci.yml - reverted change to `pull-submodules`
lobis Mar 24, 2022
dc20190
Revert "cmake - removed old `FindGarfieldOld.cmake`"
lobis Mar 25, 2022
8902794
Testing - better structure for test file
lobis Mar 25, 2022
ddf1a6d
Testing - updated executable name
lobis Mar 25, 2022
8acb468
Removed old `testing` directory as it is now obsolete
lobis Mar 25, 2022
2f3b4b7
Updated testing cmake to reduce code replication
lobis Mar 25, 2022
b7ca972
New Test for TRestMetadata
DavidDiezIb Mar 26, 2022
b3c32b4
Remove commented line
DavidDiezIb Mar 26, 2022
d06a89f
Testing - formatting
lobis Mar 27, 2022
4104604
Testing - refactored testings by @DavidDiezIb
lobis Mar 27, 2022
337138a
cmake/Testing.cmake - removed msg
lobis Mar 27, 2022
b6efb43
Testing - fixed error in test
lobis Mar 27, 2022
bf592d3
Merge branch 'cmake-ctest' of github.com:rest-for-physics/framework i…
lobis Mar 28, 2022
6f85645
Merge remote-tracking branch 'origin/master' into cmake-ctest
lobis Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/framework/test/files/metadata.rml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<metadata p1="75" p2="12.32" p3="Aloha" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

25 changes: 25 additions & 0 deletions source/framework/test/src/FrameworkCore.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

#include <TRestRun.h>
#include <TRestMetadata.h>
#include <gtest/gtest.h>

#include <filesystem>
Expand All @@ -10,6 +11,7 @@ using namespace std;

#define FILES_PATH fs::path(__FILE__).parent_path().parent_path() / "files"
#define BASIC_TRESTRUN_RML FILES_PATH / "TRestRunBasic.rml"
#define BASIC_TRESTMETADATA_RML FILES_PATH / "metadata.rml"

TEST(FrameworkCore, TestFiles) {
cout << "FrameworkCore test files path: " << FILES_PATH << endl;
Expand Down Expand Up @@ -37,3 +39,26 @@ TEST(FrameworkCore, TRestRun) {
EXPECT_TRUE(run.GetRunDescription() == "This is a test for TRestRun");
EXPECT_TRUE(run.GetVerboseLevelString() == "debug");
}

TEST(FrameworkCore, TRestMetadata) {
// Check required files exist
EXPECT_TRUE(fs::exists(BASIC_TRESTMETADATA_RML));

// Create new TRestMetadata class
class metadataTestClass: public TRestMetadata{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any REST class should be starting by TRest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any REST class should be starting by TRest

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

Copy link
Member

@jgalan jgalan Mar 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@lobis lobis Mar 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I am not debating that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

int fP1;
double fP2;
string fP3;
};

metadataTestClass meta;
meta.LoadConfigFromFile(BASIC_TRESTMETADATA_RML, "metadata");

meta.PrintMetadata();
//cout << meta.GetParameter("p1") << endl;

EXPECT_TRUE(meta.GetParameter("p1") == "75");
EXPECT_TRUE(meta.GetParameter("p2") == "12.32");
EXPECT_TRUE(meta.GetParameter("p3") == "Aloha");

}